Skip to content
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

Update math macros to avoid computing callable arguments more than once #391

Closed
wants to merge 1 commit into from
Closed

Conversation

Keating950
Copy link

Currently, if you pass an a callable argument to (e.g.) the max macro, the argument will be computed more than once. max(expensive_function(a), expensive_function(b)) will expand to:

expensive_function(a) > expensive_function(b) ? expensive_function(a) : expensive_function(b)

The compiler can't always optimize these calls away, as seen in the Godbolt disassembly here. This PR uses the GCC typeof extension to avoid these unnecessary calls where possible, falling back to the existing macros should that feature be unavailable. (Testing for it with the latest Arduino SDK shows that the extension is available.)

Comment on lines +123 to 125

#define radians(deg) ((deg)*DEG_TO_RAD)
#define degrees(rad) ((rad)*RAD_TO_DEG)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two lines (124, 125 for radians and degrees) expose the very same problem as do all macro calls. Why not define them as inline constexpr functions instead? Like so:
'
template<typename T> inline constexpr T min(T a, T b) { return a > b ? b : a; }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because that doesn't work in C, and these macros are currently usable from C, so that would break compatibility.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also concerned about binary size.

@matthijskooijman
Copy link
Collaborator

Note that min/max have already been updated similarly to what you do in this PR (and with a template function for C++) in the
https://github.com/arduino/ArduinoCore-API repository. That repo will be used as a basis for the common code in this repo in the (near?) future, so that would be the best place to make fixes like these (I don't think changes to the common code will be merged here anymore).

As I said, just min and max have been fixed in that repo, the others are still up for improvement. See arduino/ArduinoCore-API#85 for discussion about this, and a partial implementation that could be used as a basis. @Keating950, if you're interested in this, maybe you could make a PR over there? The -API repo is already used by the megaavr and SAMD boards, so it's easy to test there, and I think there's also a branch in this repo that used the -API repo (but it might be a bit old).

@Keating950
Copy link
Author

That sounds like the best place to start. Thanks for the direction!

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.

3 participants