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

Switch one PI to M_PI #2

Closed
wants to merge 1 commit into from
Closed

Conversation

eddelbuettel
Copy link

Dear Henrik, Dear fddm team,

The Rcpp team is trying to move towards defining STRICT_R_HEADERS by default. Please the issue ticket at RcppCore/Rcpp#1158 for motivation and history.

Your package uses (in one file) PI (instead of the standard C define M_PI) and PI goes away when we set STRICT_R_HEADERS (as a #define in a header or source file, a -DSTRICT_R_HEADERS as a compiler flag, or as a #define in the Rcpp sources as we currently do). We plan to enable STRICT_R_HEADERS by the Jan 2022 release of Rcpp, and will likely offer you a define to suppress it. So if you really do not want the change you can prevent it -- see these lines in Rcpp for details:
https://github.com/RcppCore/Rcpp/blob/e79c70e76bc2a776d2d57287f7192dbdbcb292aa/inst/include/Rcpp/r/headers.h#L28-L38

This very simple PR changes PI to M_PI. Your code will then work with and without STRICT_R_HEADERS (as M_PI is an old define from C). PI only works when STRICT_R_HEADERS is not defined.

As discussed in RcppCore/Rcpp#1158, this is not urgent, but we of course welcome relatively prompt resolution at CRAN so when we continue to test for this (at a likely montly pace) so we do not get false positives as we will continue to use the CRAN set of packages (as opposed to hand-curated set of upstream dev versions).

Many thanks for your help, and I hope you continue to find Rcpp helpful. Please don't hesitate to ask if you have any questions.

@kendalfoster
Copy link
Collaborator

Thanks for pointing that out! However, we will not accept the pull request and instead make a different change. The use of PI there is actually a bug from our side (thanks a lot for noticing this!) as we have defined our own pi elsewhere (PI_CONST). So we will simply replace all uses of PI with our correct one. So even though we close this now, you can consider this issue as resolved. Please let us know if you see something else cropping up or for some reason we do not manage to resolve this on our own appropriately.

@eddelbuettel
Copy link
Author

Thanks for the prompt response and sounds good to me: anything but PI goes!

I have been a little to eager in filing as resolved 'once merged' and learned that I should really only consider it resolved once an updated package is on CRAN but I am sure you will get it there promptly. If not I may nag once or twire :)

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.

2 participants