Skip to content

Calls to UNPROTECT() to quiet rchk #949

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 16, 2019
Merged

Calls to UNPROTECT() to quiet rchk #949

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d60c31
Calls to UNPROTECT() to quiet rchk
romainfrancois Mar 13, 2019
d76ff05
++nprot is more idiomatic
romainfrancois Mar 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions inst/include/Rcpp/macros/macros.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,11 @@
#ifndef BEGIN_RCPP
#define BEGIN_RCPP \
int rcpp_output_type = 0 ; \
int nprot = 0; \
(void)rcpp_output_type; \
SEXP rcpp_output_condition = R_NilValue ; \
(void)rcpp_output_condition; \
static SEXP stop_sym = Rf_install("stop"); \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nicer if stop could be installed only once, instead of once per function, but that's an improvement I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjallaire would it make some sense to have something like :

SEXP stop_symbol() {
  static SEXP s = Rf_install("stop");
  return s;
}

somewhere at the top of the generated RcppExports.cpp so that the stop symbol is only installed once, instead of once per function ?

Copy link
Contributor

@lionel- lionel- Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might also need to make this function static in order to support unloading DLLs on certain platforms. This is not important for packages but is important for sourceCpp(). IIRC, it caches dynlibs and those can be unloaded and reloaded several times in the same R session. On certain platforms, the ctors for the static variables failed to be called when the dynlib was loaded again, even though the dtors did get called on unloading, which caused crashes. I fixed this by giving static scope to the functions wrapping the static variables.

try {
#endif

Expand All @@ -48,26 +50,30 @@
catch(Rcpp::exception& __ex__) { \
rcpp_output_type = 2 ; \
rcpp_output_condition = PROTECT(rcpp_exception_to_r_condition(__ex__)) ; \
++nprot; \
} \
catch( std::exception& __ex__ ){ \
rcpp_output_type = 2 ; \
rcpp_output_condition = PROTECT(exception_to_r_condition(__ex__)) ; \
++nprot; \
} \
catch( ... ){ \
rcpp_output_type = 2 ; \
rcpp_output_condition = PROTECT(string_to_try_error("c++ exception (unknown reason)")) ; \
++nprot; \
} \
if( rcpp_output_type == 1 ){ \
Rf_onintr() ; \
} \
if( rcpp_output_type == 2 ){ \
SEXP stop_sym = Rf_install( "stop" ) ; \
SEXP expr = PROTECT( Rf_lang2( stop_sym , rcpp_output_condition ) ) ; \
Rf_eval( expr, R_GlobalEnv ) ; \
++nprot; \
Rf_eval( expr, R_BaseEnv ) ; \
} \
if (rcpp_output_type == 3) { \
Rcpp::internal::resumeJump(rcpp_output_condition); \
}
} \
UNPROTECT(nprot);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This never gets called, but it might quiet rchk

#endif

#ifndef END_RCPP
Expand Down