-
-
Notifications
You must be signed in to change notification settings - Fork 218
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
Header cleanup with C++11 as a baseline #1364
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.
More stuff:
- I think that functions
canUseCXX0X
,RcppCxx0xFlags
,Cxx0xFlags
can be dropped fromR/RcppLdpath.R
. - Also the
cxx0x
that is passed around in other functions. - Also the
inst/discovery
script. - Maybe even drop all the suff in
R/RcppLdpath.R
? These functions have been deprecated for some time now. - Plugins
cpp98
andcpp0x
should be dropped fromR/Attributes.R
. Probablycpp11
too. inst/include/Rcpp/exceptions
may be just flattened out after removingcpp98
.- All the
inst/include/Rcpp/generated
stuff may be removed if there are variadic alternatives. If not, that's meat for separate PRs. - The same applies to the generated stuff under
inst/include/Rcpp/module
. inst/include/Rcpp/sugar/block
seems like a good candidate to simplify too, but I'm not sure what's this code for.- Same for
inst/include/Rcpp/sugar/functions/mapply
, and we could get a variadic mapply not bounded by 3 arguments. But this requires more work. Maybe for another PR.
Regarding the "More Stuff" comment: Yes. Of course. But "small steps" and "increments" and "Rome != oneDay". PS To a first approximation each single bullet up there is a PR and reverse depends check. Some packages still use |
We cannot address all possible changes in a PR. I like the amount of change and hence potential instability here and will likely try to draw a dotted and test this at the current stage via a reverse depends run. And then proceed with, say, a variadic template if/else cleanup at a time hopefully leading to one coherent PR. Does that work for you? |
Sure, just documenting further steps that I've seen while looking around, which, as I said, can be part of other PRs. |
Maybe we should think about using the wiki, or discussions, or .... $INSERT_WHATEVER_HERE ... as comments deeply nested in one of hundreds of PR tend to get lost. |
If #1363 is supposed to cover several PRs, I can move the comments there. |
That may work. I usually think of a 'one issue -- one pr' correspondance but we don't have to. Or maybe even open a new issue just to hold comments for possibly multiple issues and prs? Cleaner still? |
There is more that can be done but each removal of a #DEFINE needs to check exactly where it is used. The bottom part of compiler.h still spans a grid over C++0x/TR1 to see if unordered map and set are a given (as in C++11 or later) which together with other calls of 'C++11 it is now' can simplify but we need to carefully walk this through the rest of the code and not just delete here as we'd keep unnecessary #define 'orphans' around.
Given that we test for C++11 we can assume variadic templates and just use the #define. Can likely be generalized to icc and clang too.
Removes one if/else use in table.h for map, none for set
af96e43
to
12c29ee
Compare
Sad to report that I am seeing a lot of 'carnage'. About eight hours in (after I did wait for the previous, more innocent PR to finish a reverse depends as 1.0.14.6) I see 373 successes but 14 failures (and the usual hardwired 4 skipped at this point). Failures are
and we have errors such as a simple brace initialization failing which I just used to bissect: Rscript -e 'Rcpp::cppFunction("Rcpp::NumericVector makevec(int n) { Rcpp::NumericVector z = {1,2,3}; return z; }")' With the ten commits here being
I can pinpoint the break between d4870af (still good) and 714c483 (no longer compiles simple example). I may stop the reverse depends and re-start there. |
So this incomplete rev dep run was under the (hoped for) 1.0.14.7 version; the last good one was 1.0.14.6 after the last PR. I am setting up a new one at the pinpointed bisection as 1.0.14.6.1. Incidentally I can cherry-pick the remaining PRs back, omitting just one commit from the sequence, and that passes the local test. So this may be a valid candidate too, tentatively named 1.0.14.6.2. I am working off a 'repair' branch called feature/header_cleanup_reset which I may push. It now has edd@rob:~/git/rcpp(feature/header_cleanup_reset)$ git ls | head -10
* 7f309e17 - (HEAD -> feature/header_cleanup_reset) Following rebase roll micro release and date, update ChangeLog (6 minutes ago) <Dirk Eddelbuettel>
* de01352d - Reflect code review comments (7 minutes ago) <Dirk Eddelbuettel>
* e87c738f - Remove HAS_STATIC_ASSERT define and simplify at its sole use (10 minutes ago) <Dirk Eddelbuettel>
* d4870afd - Reduced compiler.h to about a page of mostly defines and includes (9 hours ago) <Dirk Eddelbuettel>
* d2c859aa - Further cleanup (9 hours ago) <Dirk Eddelbuettel>
* 39f3c422 - No longer provide fallback for unordered_{set,map} which are given (9 hours ago) <Dirk Eddelbuettel>
* 2665b4d3 - Expand macos matrix to macos-13, simplify via r-ci with bootstrap (9 hours ago) <Dirk Eddelbuettel>
* f1956608 - Do not rely on __has_feature() for g++ (9 hours ago) <Dirk Eddelbuettel>
* 1d9a5497 - First step of simplifying compiler checks (9 hours ago) <Dirk Eddelbuettel>
* 0905d92e - (origin/master, origin/HEAD, master) Use lsInteral3 instead of lsInternal (#1362) (10 hours ago) <Dirk Eddelbuettel>
edd@rob:~/git/rcpp(feature/header_cleanup_reset)$ which is the above up to d4870af, omitting the identified 714c483 and then re-adding the remaining three (via cherry-pick so yields new sha1). |
Oh I am smelling the bacon -- I overlooked one use of one of those three defines: Rcpp/inst/include/Rcpp/vector/Vector.h Lines 239 to 243 in 12c29ee
That would do it. Seems like I can resurrect this branch and PR. PS Now 2 1/2 hours in with 120+ packages passed and on issues so far. PPS Now 12 hours in, 600+ packages, zero issues so far. Looks good. |
This can likely be merged in a day or two once the run is done to form the anticipated 'base station' from which we can launch further targeted cleanups, notably simplifying the use of variatic templates. I should be able to get to that by mid-week. |
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!
Rcpp has worked very hard for very long to provide powerful idioms irrespective of the compiler encountered. This was mostly due to 'modern C++' coming around very slowly when Rcpp got going. It is now 2025, and C++11 itself starts to feel like 'legacy code' so we can move forward and discard accommodations for compilation standards older than C++11.
This will allow us to remove a number of branches from numerous if/else blocks and remove included generated code. Last years nice and very careful PR #1303 already went that way, we can now go further. This PR starts by making the platform/compiler.h header simpler by removing the various tests: if C++11 is given, a number of things can be asserted and defined. We can build on it and examine the different defines and one-by-one remove conditional use ... and eventually the define. (While being careful. Invariably one or two client packages may be using the define.)
Checklist
R CMD check
still passes all tests