Use of macros for math functions causes unexpected results when argument has side effect

[iacopobaroncini]

Hi folks,

I know it has been discussed already here and there...

I find semantically wrong the fact that the following math functions are implemented as macros: abs, min, max, constrain, round... and let me add bitWrite to the list.

It is well known that using these function the way the rest of the world does, introduces faults, and therefore generates failures.

Writing something like *abs(cos(x) - 123 sq(sin(x)) would introduce redundant calculations, just a crazy delay in this case... Writing something like abs(a += b) or abs(i++) just introduces a fault**... it does something different (wrong in fact).

Assuming that using template and inline functions would solve the issue with no extra executable space taken, I would like to know if there is any other good reason why those dangerous macro definitions are still in force.

Kind regards, Iacopo. #

[PaulStoffregen]

I can confirm this multiple expression evaluation problem has come up for people using Teensy.

The solution is not so simple, because the C++ std namespace also has min & max, and bringing in those headers causes all sorts of other compatibility headaches. These functions/macros are also needed for plain C, which is seldom used in the Arduino world, but failure to consider it will break some programs.

Here's a conversation from earlier this year about some of the details:

https://forum.pjrc.com/threads/44596-Teensyduino-1-37-Beta-2-(Arduino-1-8-3-support)?p=145150&viewfull=1#post145150

[iacopobaroncini]

All crear Paul. Many thanks for your explanation. I actually had problems (with abs and round in first instance) while working with Arduino boards (micro and mega).

[cousteaulecommandant]

Maybe make both a templated function and a macro that calls the templated function? That would avoid the multiple call issue AND the namespace clash.

template<typename T> T &Max(T &a, T &b) { return (a>=b) ? a : b; }
template<typename T> const T &Max(const T &a, const T &b) { return (a>=b) ? a : b; }
#define max(a, b) Max((a), (b))

Some related discussion (about round() specifically): arduino/ArduinoCore-API#76

[matthijskooijman]

(Note: The first half of this post is somewhat irrelevant, since what I suggest here was already implemented, so feel free to skip the first half)

Having a macro that calls a template function does not actually fix the namespace clash. Consider

#define max(a, b) Max((a), (b))

and later:

return std::max(1, 2);

The `std::max`` will still trigger the macro, breaking the build. I would say that there should not be macros for this at all (macros are fragile in general, but especially when they use names that are so likely to be used elsewhere too), just use expose the C++ templated function directly. This does need C++ templates to make sure the typing stays more or less the same as the current macros.

For C, that means there are two options:

• Keep the macros. Not ideal, but in C, the chances of collisions are a lot smaller (since you cannot have e.g. a max function in another namespace, or a class, etc. and variables do not trigger the macro).
• Pick a single (big) numerical type and provide a single min/max variant using that type. This is not quite good, since there is no type that can cover both big integers and big double values, so you'd end up losing range or precision either way).

Already implemented

I was halfway writing an example of how such a template function could look like, when I noticed that @facchinm actually already implemented exactly what I was suggesting above for the min and max functions:

https://github.com/arduino/ArduinoCore-API/commit/30e439755e47339d19a835da9cd79cb1791c2e9f#diff-b61d0ca976d0e73288ab501219df735a

Even more, he added a nice trick for the C macro version using "statement expressions" to store both parameters in local variables to prevent double evaluation.

Left to implement

So I think that this issue is already solved for the min and max functions, but the same fix should still be applied to the other macros (constrain, radians, degrees, sq, maybe also lowByte, highByte, bitRead, bitSet, bitClear, bitWrite, bit and word for good measure). Also note that https://github.com/arduino/ArduinoCore-avr/issues/324 mentions abs and round, which are apparentely defined by the AVR core, but not here? Is that intentional?

[matthijskooijman]

I implement some of these other functions in the template+macro style for another project here: https://github.com/ianfixes/arduino_ci/pull/144/commits/5fce911fb27e49e794b3d808ccbb1b5efdfc784e Those could serve as a basis for fixing them here too.

[matthijskooijman]

The math macros came up in the AVR core again (though for somewhat unrelated reasons) and prompted some discussion about changing the abs macro.

First off, let me repeat a question from a previous comment: abs and round, are currently defined by the AVR core, but not here. On ArduinoCore-SAMD, it seems abs is defined in the core-specific Arduino.h instead of in the -API Common.h like the other macros. Also, it seems that -SAMD currently does not offer round at all? Are these discrepancies intentional? Should abs and round be defined by -API for consistency? @facchinm, something you could comment on?

If they should, then I think they should also be converted to template functions, falling back to macros for C only, like the other functions mentioned in this issue.

Additionally, for abs, I would suggest changing the expression from ((x)>0?(x):-(x)) to `((x)>=0?(x)+0:-(x)) because:

1. Using >= makes more sense than >, no need to invert 0
2. Using +0 ensures -0.0 becomes 0.0
3. Using >= produces slightly shorter code on AVR than > (bonus benefit)

See https://github.com/arduino/ArduinoCore-avr/pull/406 for additional discussion.