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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

Keating950 wants to merge1 commit intoarduino:masterfromKeating950:master

Closed

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

Keating950 wants to merge1 commit intoarduino:masterfromKeating950:master

Conversation

Copy link

Keating950 commentedJan 23, 2021

Currently, if you pass an a callable argument to (e.g.) themax 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 theGodbolt disassembly here. This PR uses the GCCtypeof 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.)

Update math macros to avoid computing callable arguments more than once

8c9947e

maaler reviewed

Jan 28, 2021

View reviewed changes

cores/arduino/Arduino.h

Comment on lines +123 to 125


		#defineradians(deg) ((deg)*DEG_TO_RAD)
		#definedegrees(rad) ((rad)*RAD_TO_DEG)

Copy link

maalerJan 28, 2021

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 doall 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

matthijskooijmanJan 28, 2021

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

Keating950Jan 29, 2021

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.

Copy link

Collaborator

matthijskooijman commentedJan 28, 2021

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, justmin andmax have been fixed in that repo, the others are still up for improvement. Seearduino/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).