| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
This PR lays the groundwork for masking unsafe Rf_error() calls by introducing a macro that generates compilation messages warning users to use Rcpp::stop() instead. This is the first step toward addressing issue #1247.
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file| File | Description |
|---|---|
| inst/include/Rcpp/macros/mask.h | New file introducing the Rf_error macro that shows a compilation message directing users to Rcpp::stop() |
| inst/include/RcppCommon.h | Includes the new mask.h header file at the end of the common includes |
| src/attributes.cpp | Protects auto-generated Rf_error call with parentheses to prevent macro expansion |
| inst/tinytest/cpp/stack.cpp | Protects test code's Rf_error call with parentheses and updates copyright |
| inst/tinytest/testRcppInterfaceExporter/src/RcppExports.cpp | Protects auto-generated Rf_error call with parentheses |
| ChangeLog | Documents all changes made in this PR |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
There was a problem hiding this comment.
Looks good to me!
Also better than my PR where @copilot always finds a typo to nag about.
Sorry, something went wrong.
|
So, as discussed, and the revdep machine made it clear, we cannot generate a note. Pragma messages are considered statements by gcc (see e.g. here and bugs linked there). Options are: be silent, or generate a warning without replacing. In order to ask CRAN about what they think, we need to calculate how many packages would generate this warning. An upper bound was provided here by just grepping the sources of all reverse dependencies: 263 packages. From these, only 24 would also fail compilation or checks as documented in #1406. To further refine this list, we could collect the install logs and grep for the pragma (are they in the revdep machine from the previous run?). |
Sorry, something went wrong.
|
This is a version that emits a message instead of a warning, and avoids the problem of inserting two statements. Could we run a revdep with this? |
Sorry, something went wrong.
There was a problem hiding this comment.
This looks good to me (modulo a micro-comment), let's see what the reverse dependencies say (in due time, now running).
(I asked copilot too which often catches little gotchas.)
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
|
First package to break compilation is BayesProject: /usr/local/lib/R/site-library/Rcpp/include/Rcpp/macros/mask.h:42:23: error: invalid use of ‘::’ [-Wtemplate-body]
42 | #define Rf_error(...) Rcpp::internal::Rf_error(__VA_ARGS__)
| ^~~~
/usr/share/R/include/R_ext/Error.h:84:15: note: in expansion of macro ‘Rf_error’
84 | #define error Rf_error
| ^~~~~~~~
/usr/local/lib/R/site-library/RcppEigen/include/Eigen/src/IterativeLinearSolvers/IterativeSolverBase.h:305:14: note: in expansion of macro ‘error’
305 | RealScalar error() const
| ^~~~~
It's an interaction with Boost Eigen and R 😿 This one also triggers the 'R.h included' message so a cleanup may fix both. |
Sorry, something went wrong.
|
Ah, of course, group 1 in #1406 is expected to fail due to the inclusion of <R.h>. But we want to fix these as soon as possible. |
Sorry, something went wrong.
|
Apologies for the back-and-forth, but the internal template thing didn't work. I thought that the message would be shown only if the template was instantiated, but that's not the case. I thought I tested it correctly, but obviously I did something wrong, because now I see messages all over the place when Rf_error is not there. So we are back at square 1, meaning two options:
|
Sorry, something went wrong.
|
Let's take (1) and merge this then? |
Sorry, something went wrong.
|
Yes. Let me catch my breath after all the excitement in #1430 and get back to you. |
Sorry, something went wrong.
There was a problem hiding this comment.
This looks good, but ... it now does nothing?
So shall we merge this as a placeholder to be activated once 'group 1' of R.h has been cleaned up?
Sorry, something went wrong.
|
Should we maybe not comment it out (as right now) but make it active behind an 'opt-in' environment variable? Then we could merge now, and we (and Kurt) can run with the variable set. Better? |
Sorry, something went wrong.
* Add a message at compilation time * Protect the valid Rf_error calls generated by Rcpp
…switching to a warning
|
Sounds good to me. Here it is with an on switch. |
Sorry, something went wrong.
|
Looks like a double env var :) That'll do. |
Sorry, something went wrong.
|
Yes, it's a great idea. CRAN may inject that flag when they're ready to process such warnings. In June the warning may be unconditional (unless disabled explicitly), then next year this may be an error. Thanks, merging here! |
Sorry, something went wrong.
|
So for completeness I ran a reverse-dependency check over what is now in the main branch as 1.1.0.13, and a single package came up with an issue: a std::min( an_int, new_nrow()) balked because we now return R_xlen_t. I made a quick PR for the affected package (see here, I also slipped in a 'vistor' to replace one ATTRIB()) so we should be good to go. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
First step to #1247
Checklist