Skip to content

Prevent abs() macro from sign-flipping 0.0F #8116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dok-net
Copy link
Contributor

@dok-net dok-net commented Jun 11, 2021

Fixes #8115

@dok-net
Copy link
Contributor Author

dok-net commented Jun 11, 2021

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; })

@earlephilhower
Copy link
Collaborator

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.

@dok-net
Copy link
Contributor Author

dok-net commented Jun 12, 2021

@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 :-)

@mcspr
Copy link
Collaborator

mcspr commented Oct 9, 2021

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

@dok-net
Copy link
Contributor Author

dok-net commented Oct 9, 2021

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

@mcspr
Copy link
Collaborator

mcspr commented Oct 9, 2021

@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:
arduino/ArduinoCore-API#76 (comment)

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

@dok-net dok-net force-pushed the fix_8115 branch 2 times, most recently from 3c084a7 to 13e62b8 Compare October 16, 2021 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

abs() macro in Arduino.h inverts signedness of (positive) 0.0F
3 participants