Macro function doesn't calculate as it should

[Pararera]

Hi guys,
I made macro function which converts kilometers into meters. Logic Works but practice does not. I get weird number.
Code is below:

#define one_km 10000000 // 1000m * 10000(one meter)


#define km2m(value) \
	value * one_km

volatile uint64_t distances[MAX_DISTANCES];



// LATER IN CODE
distances[TOTAL_DISTANCE] = km2m(1234);

Output I should get: 12340000000
Output I get: 18446744073164649728

But if I do next:

distances[TOTAL_DISTANCE] = 12340000000;

I get right output. Why it does this? Thanks!

[TheIdeasMan]

Hi,

Some guidelines:

Prefer not to use macros: https://stackoverflow.com/questions/14041453/why-are-preprocessor-macros-evil-and-what-are-the-alternatives ;
Prefer not to use arrays, use a std::vector instead.

Apart from that:

#define one_km 10000000 // 1000m * 10000(one meter)

No, you are doing tenths of millimetres.

#define km2m(value) \
	value * one_km 

There is integer division there.

distances[TOTAL_DISTANCE] = km2m(1234);

Putting the total in the array is weird, it should be it's own variable.

Was TOTAL_DISTANCE initialized to anything?

[Peter87]

There is integer division there.

No, that's not a division. It's how you write if you want to split a macro definition over multiple lines.

Output I get: 18446744073164649728

The problem is that the integers overflow. Using long long for the integer literals should give you the expected result.

#define one_km 10000000LL

// LATER IN CODE
distances[TOTAL_DISTANCE] = km2m(1234LL);

[coder777]

On kilometer are actually 1000 meters thus 1234 km is 1234000 m.

I suggest that you replace the macro with template:

const auto one_km 1000 // meter

template<typename value_type>
value_type km2m(value_type value)
{
  return value * one_km;
}

[Pararera]

@Peter87 Thanks! That was the problem!
@coder777 One meter(lets say on display) is 10000 in memory. 1.5m is 15000 in memory. With this way I can have precision with integers. I hope you get it why one meter is 10000

[Pararera]

@Peter87
It does work but now I have problems with other macro functions.

#define one_km 10000000LL // 1000m * 10000(one meter)
#define one_m2ft 32808399LL

#define m2ft(value) \
	value * (one_m2ft / one_km)


distances[i] = m2ft(distances[i]);

Output I should get: 40485564366
Output I get: 37020000000

Should I use inline functions instead?

[keskiverto]

Now you have the integer division.
The 32808399LL / 10000000LL equals 3, does it not?

Therefore, your macro effectively expands to:
distances[i] = distances[i] * 3LL;

On the other hand, if you:
distances[i] = (distances[i] * one_m2ft ) / one_km ;
Could that overflow?

Macro, function, function template ... do not solve that issue.


As said before, macros are (mostly) unnecessary in C++:

constexpr auto one_km   = 10'000'000LL;
constexpr auto one_m2ft = 32'808'399LL;

[TheIdeasMan]

I would prefer to do the calculations with doubles including any scaling for the screen, then cast them if necessary to int. Do this with functions that have meaningful names, the programs intent will be much clearer. The code is confusing at the moment. Code should be easy to understand, even for those that may not know much about the topic. I am an Engineering Surveyor, and I found it confusing until you explained what you were doing.

Use the constant value of 25.4mm per inch to convert between metric and imperial, multiplying or dividing as necessary. Values like 32'808'399LL are not exact.

[Pararera]

I prefer int instead double because value it stored in EEPROM(Arduino). That's the reason why one meter is 10000 in memory. Everything works fine for metric system, but imperial doesn't work as it should. It's confusing me.

[Ganado]

You can store it as an int and still cast to double when doing division. Otherwise it will do integer division, like the others said.
value * (static_cast<double>(one_m2ft) / one_km)

[Pararera]

I fixed it by adding (float) in macro functions. Problem was that Imperial units aren't round as metric units(eg 1mi has 5280ft; 1km has 1000m). What's why conversion didn't work.
Now I can convert without any problem. Only I lose about 15m with every conversion. That's not big problem because user never should change system unit. But I added that option.

@EDIT:

What
static_cast<double>
means? What it does?

[keskiverto]

C has one cast syntax: (double)one_m2ft
See https://www.tutorialspoint.com/cprogramming/c_type_casting.htm

C++ has several casts. The static_cast<double>(one_m2ft) is appropriate here.
See: https://www.tutorialspoint.com/cplusplus/cpp_casting_operators.htm

The advantage of the four *_cast is that they are more specific.
Furthermore, it is really easy to find them from code, unlike the C-style cast.

[Ganado]

I'm simplifying a bit (I think), but static_cast<double>(variable) is the equivalent of doing (double)variable.

One reason to prefer static_cast<T>(variable) as opposed to (T)variable is it is much easier to search for, after the fact (you can easily search "cast"). It's very hard to search for a C-style cast, making refactoring harder.

You mentioned you get inaccuracies of around ~15m. Try doing (double) instead of (float), though I'm not sure if it will make a big difference, but doubles have double the precision of floats on most platforms.

[TheIdeasMan]

Look, it's simple:

constexpr double m2ft = 0.3048;

double MetresToFeet(const double metres) {
 
  return metres * m2ft;
}

If you want to multiply the value returned by the function by 10'000'000.0 , then cast it to int, it will still be accurate.

I am not sure why students seem to lock on to the worst way of doing something, and fail to consider something a simple as this code.

[TheIdeasMan]

I prefer int instead double because value it stored in EEPROM(Arduino). That's the reason why one meter is 10000 in memory. Everything works fine for metric system, but imperial doesn't work as it should. It's confusing me.

This sounds like an XY problem. Is it an actual requirement to store 1 metre as 10000 in memory, or is that your solution to a problem that is at the moment unknown to us.

Why is the value stored in EEPROM? What are these metre values going to be used for?

Arduino seems to have a double type (and the cast function int() ), can you explain why it needs to be an int everywhere? I am having difficulty as to seeing why using double for the calculations, then casting to int as the very last step would not work.

[keskiverto]

Isn't "Arduino" a language of its own that merely has loaned some bits from C and C++?
Wiki says: "8-bit hardware". A "double" or "long" can't be much more there than "smaller" data types.

You should have mentioned the Arduino up front and thus save yourself from advice that does not apply to your system.

[mbozzi]

From https://www.arduino.cc/en/Main/FAQ :

Can I program the Arduino board in C?
In fact, you already are; the Arduino language is merely a set of C/C++ functions that can be called from your code. Your sketch undergoes minor changes (e.g. automatic generation of function prototypes) and then is passed directly to a C/C++ compiler (avr-g++). All standard C and C++ constructs supported by avr-g++ should work in Arduino. For more details, see the page on the Arduino build process.

[Thomas1965]

One reason to prefer static_cast<T>(variable) as opposed to (T)variable is it is much easier to search for, after the fact (you can easily search "cast"). It's very hard to search for a C-style cast, making refactoring harder.

Why would I want to search for "cast" ?

In 20+ years I never had a need for it.

[Ganado]

Yeah, that's a good point. I've probably only done it once or twice to fix a problem I didn't create. The thing about C++ is that well-written C++ requires less casting than C, so it is less of an issue.

Maybe, because static_cast is so ugly and so relatively hard to type, you're more likely to think twice before using one? That would be good, because casts really are mostly avoidable in modern C++.

I agree with Stroustrup's points on using static_cast, though:
http://www.stroustrup.com/bs_faq2.html#static-cast

It's more than just the potential of code searching; he's saying it increases readability in making sure the intention is clear, and static_cast is more restrictive than (oldstyle) cast.

But for something simple like casting an int to double for division... it hardly makes a difference, so not really worth the time to worry about it.