-
-
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
Abort on R type error if debugging #860
Conversation
inst/include/Rcpp/r_cast.h
Outdated
Rf_type2char(TYPEOF(x)), | ||
Rf_type2char(RTYPE)); | ||
abort(); | ||
#elif |
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.
#else instead of #elif?
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.
Yes... whoops. But it worked with clang, so I missed it.
Codecov Report
@@ Coverage Diff @@
## master #860 +/- ##
======================================
Coverage 90.3% 90.3%
======================================
Files 70 70
Lines 3261 3261
======================================
Hits 2945 2945
Misses 316 316
Continue to review full report at Codecov.
|
inst/include/Rcpp/r_cast.h
Outdated
@@ -67,9 +67,17 @@ namespace Rcpp { | |||
default: | |||
const char* fmt = "Not compatible with requested type: " | |||
"[type=%s; target=%s]."; | |||
#ifndef NDEBUG | |||
fprintf(stderr, |
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.
Should that not be REprintf()
?
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.
I wasn't aware of REprintf
, made the change.
I used the following to test:
Rcpp::cppFunction('void type_err(IntegerVector iv) {}')
type_err("a")
Which aborts with the appropriate error message if I add -UNDEBUG
in CXXFLAGS
before compilation, otherwise gives normal Rcpp behavior.
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.
Thanks. REprintf()
is one of those gems hidden away in WRE which "they" to nag you on submission. You know, like 'no stdout, no abort, ...' ;-)
Following discussion in #857 , this PR prints an error to standard error, and aborts, allowing a debugger to examine state leading up to the error, instead of dropping out of R. The
assert
macro didn't seem like the right tool, as it would never give an informative error message. The glibcassert
macro callsabort
, so I used it here, while checking for absence ofNDEBUG
. The default build process in R is done with-DNDEBUG
so this should not alter behavior for anyone who hasn't explicitly used-UNDEBUG
, and therefore this should not impact any packages built for CRAN in the usual way.There is no obvious way for me to add an automated test for this code, since it requires compiling with and without the
NDEBUG
flag set.