-
-
Notifications
You must be signed in to change notification settings - Fork 219
Refactor some R_ext includes #1418
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
|
Thanks for the prompt PR! The CI check from the 'dev' container probably may mean nothing as the update is weekly and may have missed the change. Let me update my local build real quick and check locally too. |
|
I missed the |
|
I am turning the reverse-depends machinery on, though I don't expect too much. Packages using Rcpp as well as headers from R will have both include directories, and we are testing under R-release so this is bound to be washed over. But one never knows and we have gotten surprises before ... |
kevinushey
left a comment
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.
LGTM. I'm a bit surprised these headers are being removed outright rather than just using a compiler pragma warning to note that they'll be removed. At least that would reduce the amount of CRAN breakage in the interim, while still giving package authors a chance to update...
|
I am equally shocked. It's plain cold breakage and I suspect the CRAN parts of R Core are not too happy either. |
|
I missed this one: Rcpp/inst/include/Rcpp/Interrupt.h Line 25 in 2b0a44b
This one is needed for |
|
I would prefer if we did that next cycle. It is entirely cosmetic and there are dozens of other cosmetic things we could be doing with the headers (the directory layout is a mess, the file naming is inconsistent etc). We (as well as "other parties") have also learned at times that moving header files around is never quite as clean and simple as you think. Now it is just a call from us to a header of theirs so as you say "should" be low-impact. But then again I had to put "should" in quotes because you never know. |
|
Reverse-depends concluded, there is not thing new (package So squash-merging ahead. |
Closes #1417
<R_ext/Callbacks.h>, not used anymore and dropped from the API.<R_ext/Visibility.h>toRcpp/r/headers.h.Checklist
R CMD checkstill passes all tests