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 #140

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Keating950
Copy link

This PR updates various math macros to avoid computing callable arguments more than once using the same technique that the max macro currently does. It also replaces them with C++ template functions when __cplusplus is defined. I originally opened this as in the ArduinoCore-avr repo, but was directed here.

❯ ./test-ArduinoCore-API
===============================================================================
All tests passed (527 assertions in 226 test cases)

Copy link
Collaborator

@matthijskooijman matthijskooijman left a 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.

api/Common.h Outdated Show resolved Hide resolved
@Keating950
Copy link
Author

Minimized the diff (I believe) as requested.

@codecov-io
Copy link

Codecov Report

Merging #140 (585f67a) into master (2af4a9c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #140   +/-   ##
=======================================
  Coverage   96.06%   96.06%           
=======================================
  Files          14       14           
  Lines         839      839           
=======================================
  Hits          806      806           
  Misses         33       33           
Impacted Files Coverage Δ
api/Common.h 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2af4a9c...585f67a. Read the comment docs.

@matthijskooijman
Copy link
Collaborator

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 (constrain until sq) down to where min and max are defined, so they can share a single ifdef __cplusplus block, and are also in a place where you do not need to interrupt the extern "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
Copy link

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.

KOLANICH added a commit to KOLANICH-libs/ArduinoCore-avr that referenced this pull request Jul 26, 2022
…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 <[email protected]>
Co-Authored-By: Martino Facchin <[email protected]>
KOLANICH added a commit to KOLANICH-libs/ArduinoCore-avr that referenced this pull request Feb 24, 2023
…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 <[email protected]>
Co-Authored-By: Martino Facchin <[email protected]>
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.

4 participants