Core library defines min, max, abs and friends as macros which may cause unexpected side effects

[ghost]

Suppose you pass an argument of a function call which has a side effect as an argument to abs() like this:

int f() { /* do some nasty things */ }

void loop() {
  ...
  int b = abs(f());
  ...
}

The abs() is defined in the Arduino.h as:

#define abs(x) ((x)>0?(x):-(x))

It is easy to see, that the function will be evaluated 2 times. Moreover, if a volatile variable is passed as an argument to such macro and the value of the variable gets changed by an interrupt handler, the result will also be unpredictable. What if the argument is a complex expression? Will the code optimization help to prevend a second evaluation of the expression? IMHO that is a good question to ask.

Using macro arguments in macro body more that 1 time is misleading and dangerous. In fact users of Arduino IDE has no way of looking into the source code to see a potential problem.

You should really review the folowing macros and either rewrite them as inline functions or document the side effects:

• min(a,b)
• max(a,b)
• abs(x)
• constrain(amt,low,high)
• round(x)
• sq(x)

[Coding-Badly]

These work correctly...
https://code.google.com/p/arduino-tiny/source/browse/cores/tiny/Arduino.h?repo=core2#112

[cousteaulecommandant]

I'm not sure of the state of the documentation when this issue was submitted, but right now it does warn you not to do that sort of thing.

However this is C++; why not take advantage of it?

template <typename T>
static inline T min(T a, T b) {
    return (a<b) ? a : b;
}
template <typename T>
static inline T max(T a, T b) {
    return (a>b) ? a : b;
}

And if we want to compare 3 or more numbers:

#if __cplusplus >= 201103L
template <typename T, typename... Targs>
static inline T min(T a, T b, T c, Targs... args) {
    return min(a, min(b, c, args...));
}
template <typename T, typename... Targs>
static inline T max(T a, T b, T c, Targs... args) {
    return max(a, max(b, c, args...));
}
#endif // __cplusplus >= 201103L

(this was a random thought; don't pay much attention to it if you think it doesn't deserve it)

[Coding-Badly]

A good discussion of each choice...
http://forum.arduino.cc/index.php?topic=84364.0

However this is C++; why not take advantage of it?

Do all Arduino programmers use C++?

The code of interest from the link above...

/*=============================================================================
  abs - return the absolute value
=============================================================================*/

/* Libc provides an abs macro for the int datattype.  Remove the macro so we 
can provide an overloaded function.  The Libc functions are used instead of a 
macro because they seem to result in a smaller image. */

#ifdef abs
#undef abs
#endif

inline int abs( int __i ) { return( __builtin_abs( __i ) ); }
inline long abs( long __i ) { return( __builtin_labs( __i ) ); }
inline double abs( double __d ) { return( fabs( __d ) ); }


/*=============================================================================
  round - round a floating-point number away from zero return a long
=============================================================================*/

/* round is just not going to work.  Like {forum user}, I have no 
idea why it works for ATmega processors.  The problem is Libc defines a round 
that is not compatible with the Arduino round and the Libc version cannot be 
overridden.  round gets the boot. */

/*
#ifdef round
#undef round
#endif

inline long round( float x ) { return( lroundf( x ) ); }
inline long round( double x ) { return( lround( x ) ); }
inline long round( long double x ) { return( lroundl( x ) ); }
*/


/*=============================================================================
  max - return the maximum of two numbers
=============================================================================*/

/* http://arduino.cc/forum/index.php/topic,84364.msg640438.html#msg640438 */

#define max(a,b) \
  ({ \
    typeof (a) _a = (a); \
    typeof (b) _b = (b); \
    _a > _b ? _a : _b; \
  })


/*=============================================================================
  min - return the minimum of two numbers
=============================================================================*/

/* http://arduino.cc/forum/index.php/topic,84364.msg640438.html#msg640438 */

#define min(a,b) \
  ({ \
    typeof (a) _a = (a); \
    typeof (b) _b = (b); \
    _a < _b ? _a : _b; \
  })


/*=============================================================================
  constrain - constrain a value between a minimum and a maximum
=============================================================================*/

/*
#define constrain(v,lo,hi) \
  ({ \
    typeof (v) _v = (v); \
    typeof (lo) _lo = (lo); \
    typeof (hi) _hi = (hi); \
    (_v < _lo) ? (_lo) : ( (_v > _hi) ? (_hi) : (_v) ); \
  })
*/
#define constrain(v,lo,hi) ( max( min(v,hi), lo ) )


/*=============================================================================
  sq - return a value squared
=============================================================================*/

#define sq(x) \
  ({ \
    typeof (x) _x = (x); \
    _x * _x; \
  })

[cousteaulecommandant]

Do all Arduino programmers use C++?

Yes, they do. They may not know, but they do, since Arduino sketches are C++.
The solution you proposed uses GCC extensions typeof and ({...}) in order to make a macro. Mine uses C++ templates and makes a static inline function (which is basically a less scary macro). Personally I like my solution more as it uses pure C++, but I'd say both solutions are valid (unless there are plans to support / move to a different compiler).

[NicoHood]

I also like @cousteaulecommandant version. This would also enable multiple params for min() etc.

[cousteaulecommandant]

A good discussion of each choice...
http://forum.arduino.cc/index.php?topic=84364.0

Interesting. They propose using some C++ trickery involving const T& for both the parameter and return types (which is a bit above my knowledge of C++), like

template <typename T>
const T& min(const T& a, const T& b) {
    return (a<b) ? a : b;
}

I don't know how good this is compared to my static inline approach. I guess the static part should be left if this is going to be in a header file, plus the inline part probably wouldn't hurt. With this, the compiler "knows" this goes inlined, so it's probably optimized anyway and the const & part might not be needed.

(In the link they explain that the const & on the parameters is for preventing them from being different types and the compiler coercing them to the same type, which I don't think is actually a useful restriction.)

PS: maybe this & trick is so that the function returns an actual reference and not just a value, and you can do things like int *c = &min(a, b)

[cousteaulecommandant]

Never mind; it seems that my function cannot be used with different types either; so I'd prefer the one proposed by maniacbug on the forum (the one with &) since it avoids copying the values (which probably doesn't matter anyway since copying numbers is not a big deal) and allows using the value as a reference (no idea if this will actually ever be used); plus it seems to be more "the C++ way". On the other hand, I'm not sure how to do this with the multi-argument version (T...&args doesn't seem to work).(never mind; fixed)
Also, maybe a version without the 3 const could be provided, which would be useful for doing stuff like min(a, b) = 0. However this will break stuff such as min(1,2) because "no conversion from int to int&", so both versions should be kept. (Not sure if this is actually useful, but right now min(a, b) = 0 is valid in Arduino, so maybe this should be provided for backwards compatibility at least.)

Comparing different types can still be achieved by either explicitly specifying it as e.g. min<int>(a, b), or converting both to the same type as min(int(a), int(b)). Maybe this should be documented.

I'd still add static inline to the declaration though, if this is going to be in a header.

So in short, the code for min would end up as

template <typename T>
static inline const T& min(const T& a, const T& b) {
    return (a<b) ? a : b;
}
template <typename T>
static inline T& min(T& a, T& b) {
    return (a<b) ? a : b;
}

#if __cplusplus >= 201103L
template <typename T, typename... Targs>
static inline const T& min(const T& a, const T& b, const T& c, const Targs&... args) {
    return min(a, min(b, c, args...));
}
#endif // __cplusplus >= 201103L
template <typename T, typename... Targs>
static inline T& min(T& a, T& b, T& c, Targs&... args) {
    return min(a, min(b, c, args...));
}
#endif // __cplusplus >= 201103L

(NB: I had a mistake in my original proposal; using T... is wrong; I have to make a new "pack type" for that. I've already edited it.)

---

By the way, did you know that Arduino doesn't seem to allow putting the template<...> on a separate line on sketches? The parser must be messing it up somehow.

[Coding-Badly]

I'd prefer the one proposed by maniacbug on the forum (the one with &) since it avoids copying the values

Have more faith in the optimizer. While it may not be perfect, it is good enough that it eliminates trivial value copying.

[cousteaulecommandant]

Have more faith in the optimizer

I do; it's just "the usual practice in C++". In fact I checked with and without & and the code size was the same, so I guess no extra instructions were actually generated. It's just this paranoia I've frequently seen in C++ that an object might actually be a huge and complex thing for which a copy might not be avoidable in some situations, so references are used a lot. (Another example is the discussion on whether to use ++i instead of i++ in for loops; it actually doesn't matter when i is an int but the ++i way seems to be preferred "just in case". C people seem to trust the optimizer more.)

Also the version with the & doesn't really affect the code size, so why not use it if it's "the C++ style" (and in fact the one std::min uses)?

[Makuna]

Something to consider, as library writers are making more complex "topics" simpler through their library API, they will use stl (containers) which requires the min and max to be undef'ed; thus any user code after it will not have the macros available to them. Further, other library included after that use the min and max macros will now not compile.

This mess needs to be cleaned up. As a library writer myself I am running into support issues due to other libraries undef'ing them.

[Floessie]

@per1234 Hint: You can find a nice modern min()/max() implementation here.