-
-
Notifications
You must be signed in to change notification settings - Fork 209
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
Allow using variadic templates instead of pre-generated code where supported #1303
Allow using variadic templates instead of pre-generated code where supported #1303
Conversation
You're the man. This has (implicitly) been on the TODO list for years ever since C++11 became the minimum compiler standard supported by R (in R 4.0.0 if I recall correctly). We should absolutely do this and I can turn the reverse depends machinery on (running on a courtesy box I have access to that is approximately as old as Rcpp itself so this takes 'forever' (two days) but it is a suitable background task). We do, as you likely know, aim for bi-annual release so in case this proves to be too invasive for the planned next release in six weeks (which is somewhat soon-ish) we cam always consider rolling to the next January and invite people to test away on the RCs). |
Brilliant, sounds like a plan! |
This PR is actually surgically precise and only extends the surfce. It really should have no implications (or so we hope ...). We can plan to go over the code with a big broom after the July release and contemplate throwing C++98 code out. Maybe starting by eyeballing how much impact / collateral damage this may have. The is likely right. |
Just to double-check for the |
Fair question. "Maybe". It's a little assymetric either way. And as long as we do accomodate pre-C++11 anyway does it hurt? I also wonder how that file got away with not having a check for C++11. Maybe it only is called in a C++11 context in which you could simplify. |
Hmm yeah I see how it could be a bit tricky. To keep this PR as simple as possible, I'll leave the generated code in place and handle it the same as the other templates. Likely easier to leave it for a future cleanup |
Just to chime back in: the tests ran, they had no run in a bit so no gain from |
Brilliant! I think the PR now includes variadic templates for all of the generated methods/functions, covering the files:
Let me know if I've missed any! |
Perfect. I will let the current test run to its end (think "days" not hours, sadly) and then add whereever we're at then. As the pull request template suggests, a ChangeLog entry would be appreciated. Feel free to choose a single day or multiple as the commits came -- that doesn't really matter. We will also summarise in inst/NEWS.Rd but I still like ChangeLog summaries. |
Sounds like a plan, will do |
@@ -1,3 +1,9 @@ | |||
2024-05-23 Andrew Johnson <andrew.johnson@arjohnsonau.com> |
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.
It's always two spaces (the small things....)
@@ -1,3 +1,9 @@ | |||
2024-05-23 Andrew Johnson <andrew.johnson@arjohnsonau.com> | |||
|
|||
* Added variadic templates to be used instead of the generated code |
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.
This usually starts with a 'where', commonly a single file. Mayne add inst/include/Rcpp/*:
as a wildcard? With or without the star?
Dont't sweat over the details, I can clean this up later I just thought I'd point it out before your next PR 😜
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.
Good to know!
That's the last of my changes/updates, so feel free to let me know if there are any other things to cleanup here - otherwise I'll wait to hear if I managed to break anything in the reverse-dependency checks!
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.
Yep. Because the (large, careful) PR came in stages and because the rev.dep machine runs slowwwwwwwwwly we are still at stage 2 of 3. I will re-start with the now complete set of changes so it'll be a few days ... but this is looking very very good, and it is both help -- and appreciated! So big thank you!
92a0f60
to
49bf0eb
Compare
@andrjohns I seem to now see rstan failing to install. Can you corrobate locally? |
Ah yep, I see where the issue is. Just a small typo, one sec |
No rush. This second run has to finish anyway but given it was |
Thanks for the fix. The second run, started after your first addition to the PR, had four packages with issues and all of them are fine with the updated PR. Yay. I am about to launch a new (final ?) run and will keep you posted in case something bubbles up. |
No new issues in third and final reverse depends run so ready to merge. Big thank you once, this is a helpful and nice PR! |
Opening this as a draft/PoC PR for feedback on the general idea/approach before I make more changes.
For users/systems with c++11 compilers, replacing the generated code in Rcpp with variadic templates could reduce the unnecessary compilation of unused overloads/operators and also remove the limitations on the number of parameters that can be used.
Naturally it would greatly reduce the code in the headers if the generated code were replaced with variadic templates entirely, but I can understand the goal of pre-c++11 compatibility so have made the template use conditional.
Let me know if this would be a useful change in general, or if I should approach things differently, and I'll work on the rest.
Thanks!
Checklist
R CMD check
still passes all tests