Skip to content
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

Calls to UNPROTECT() to quiet rchk #949

Merged
merged 2 commits into from
Mar 16, 2019

Conversation

romainfrancois
Copy link
Contributor

See discussion in #948

(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

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.

} \
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

@romainfrancois
Copy link
Contributor Author

FWIW, this seems to have removed the rchk warnings about potential imbalance in the RcppExports.cpp: https://builder.r-hub.io/status/original/dplyr_0.8.0.9008.tar.gz-8bd8fdcb05894674a74d5d32e57dd04a

There is still a lot of noise, some of which is dealt with in #947

@eddelbuettel
Copy link
Member

Thanks for the update.

A reverse depends check is running, but I do not have very good resources for those now (maybe a company like RStudio would like to help there, hint, hint, ...) so it will take a moment.

@coatless
Copy link
Contributor

@eddelbuettel at some point we probably should talk more about doing revdep on the Illinois Campus Cluster (ICC) with prrd.

romainfrancois added a commit to tidyverse/dplyr that referenced this pull request Mar 15, 2019
romainfrancois added a commit to tidyverse/dplyr that referenced this pull request Mar 15, 2019
* check_slice_result() no longer allocate, but protect anyway. rchk.

* protect result of quo.env(). rchk

* PROTECT quosure.expr(), as validate_unquoted_value() might alllocate

* some more rchk protection

* simplify get_date_classes() and get_time_classes()

* need (SEXP)

* some more rchk

* protect results of vec_names_or_empty()

* SymbolVector::init() protects its argument. rchk

* more protection in DataMask::setup()

* more rchk

* using setAttrib() / getAttrib() instead of Rcpp's attr() to skip 💵 for Rf_install()

* more rchk

* more rchk.

* more rchk

* more rchk

* simplify make_lambda_quosure

* get_uniques() is never used

* protect column_names. rchk

* skip copies

* protect result of wrap before the List ctor.

* using r_match() instead of SymbolVector.match_in_table()

* protect Rf_getAttrib(x, symbols::names)

* more rchk

* using Rcpp::export(rng = false).

closes #4278

* rchk says the dtor of N_Distinct might allocate

* scope the shared_ptr<> for rchk

* protect from ~Gatherer() dtor. rchk

* Cannot dirtectly send the result of match_in_table() in IntegerVector because of the unsafe ctor. rchk

* trying RcppCore/Rcpp#949 to see if it helps quiet some rchk 😱

* - Remote

* rm unused nprot

* Shield<> before CharacterVector ctor, until RcppCore/Rcpp#947 gets merged in Rcpp.

* Revert "- Remote"

This reverts commit 72bb9aa.

* SymbolMap(DataFrame) ctor to avoid rchk issue with the SymbolMap(CharacterVector) ctor

* Forbid copy construction of GroupedDataFrame and SymbolMap

* limit copies of SymbolVector

* 🏃 a branch that incorporates changes from both Rcpp PRs:

RcppCore/Rcpp#947
RcppCore/Rcpp#949

* use CRAN Rcpp
@eddelbuettel eddelbuettel merged commit b351798 into RcppCore:master Mar 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants