Arduino icon indicating copy to clipboard operation
Arduino copied to clipboard

Prevent abs() macro from sign-flipping 0.0F

Open dok-net opened this issue 3 years ago • 6 comments

Fixes #8115

dok-net avatar Jun 11 '21 09:06 dok-net

This variant solves the abs(-0.0F) := -0.0F problem, at the expense of a few CPU cycles but also 32 bytes code:

#define abstest(x) ({ __typeof__(x) _x = (x); _x >= 0 ? _x + 0 : -_x; })

dok-net avatar Jun 11 '21 19:06 dok-net

In the original PR in the AVR, I fail to see the actual bug they're solving. If -0.0f == 0.0f then why would one care?

Since this is a macro (thanks, Arduino) it seems like it will add 32 bytes per call, everywhere, with the change, which seems large.

earlephilhower avatar Jun 11 '21 20:06 earlephilhower

@earlephilhower Within a single compilation unit, the compiler seems to optimize that overhead. But as you can tell from me not putting it into the PR, instead just giving it a showing in a remark, I don't see much need for that myself. As for this PR, I find some merit in having all-zero bits representation at least for the positive zero, such that (uint32_t)0 and (float)0 are the same in memory. For what it's worth :-)

dok-net avatar Jun 12 '21 06:06 dok-net

There's also another approach - just use compiler built-ins to implement overloading, as floats also include things like inf and nan. https://gist.github.com/mcspr/8b9a317b1cbe27085c63a1e7a885902e (and pretty much re-implement std::abs, so c macro matches the current c++, or leave the long as-is and only override floats)

Not sure of code generation though, but it seems more of actually solving the issue instead of working around a certain part. ref. https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005fchoose_005fexpr and https://clang.llvm.org/docs/LanguageExtensions.html#builtin-functions

mcspr avatar Oct 09 '21 13:10 mcspr

@mcspr This is not as much about a more perfect abs() function, as it is about being Arduino-alike.

dok-net avatar Oct 09 '21 17:10 dok-net

@mcspr This is not as much about a more perfect ab() function, as it is about being Arduino-alike.

then, I'd agree with the comment above - no point in fixing anything, as neither abs or round macros are provided in the current ArduinoCore API headers in favour of math.h: https://github.com/arduino/ArduinoCore-API/issues/76#issuecomment-781132117

and current macro can stay as-is, until the issue above is definitely closed and there's a clear answer what arduino side want to do with them

mcspr avatar Oct 09 '21 19:10 mcspr