-
-
Notifications
You must be signed in to change notification settings - Fork 133
Update math macros to avoid computing callable arguments more than once #140
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for submitting this.
I left one small comment inline, and I would suggest moving all this code a bit further down, to where the current master defines min and max, in order to both minimize the diff (your current PR moves the definitions of min/max, which increases the diff) and to make sure this code is not inside the extern "C" block, removing the need for closing and reopening that block, which makes things cleaner as well.
Minimized the diff (I believe) as requested.
codecov-io
commented
Feb 17, 2021
Codecov Report
@@ Coverage Diff @@ ## master #140 +/- ## ======================================= Coverage 96.06% 96.06% ======================================= Files 14 14 Lines 839 839 ======================================= Hits 806 806 Misses 33 33
Continue to review full report at Codecov.
|
Thanks! It's not exactly as I suggested yet, because:
- You added indentation to the min/max macros, so, there's still a (IMHO unneeded) diff there. I would remove the indent again so these macros are not changed at all.
- I also suggested (at least tried to) to move the rest of the functions and macros (
constrainuntilsq) down to whereminandmaxare defined, so they can share a singleifdef __cplusplusblock, and are also in a place where you do not need to interrupt theextern "C"block. - It seems one of the commits also removes some trailing whitespace in from the two
template<class T, class L>lines for the min/max template functions, I'd also either revert that, or put it in a separate commit.
Ideally, you'd also squash all your commits together into a single commit (except maybe for whitespace-only changes, those would better be in separate commits), not sure if you're familiar with merging commits / rewriting history in git?
CLAassistant
commented
Apr 9, 2021
CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.
...texpr functions in the case of C++. Required in order to avoid conflicts to C++ stdlib. Incorporates some of the changes from arduino/ArduinoCore-API#140 and arduino/ArduinoCore-API@30e4397 Co-Authored-By: Keating Reid <keating.reid@protonmail.com> Co-Authored-By: Martino Facchin <m.facchin@arduino.cc>
...texpr functions in the case of C++. Required in order to avoid conflicts to C++ stdlib. Incorporates some of the changes from arduino/ArduinoCore-API#140 and arduino/ArduinoCore-API@30e4397 Co-Authored-By: Keating Reid <keating.reid@protonmail.com> Co-Authored-By: Martino Facchin <m.facchin@arduino.cc>
This PR updates various math macros to avoid computing callable arguments more than once using the same technique that the
maxmacro currently does. It also replaces them with C++ template functions when__cplusplusis defined. I originally opened this as in the ArduinoCore-avr repo, but was directed here.