Skip to content

Conversation

@jjallaire
Copy link
Member

Use RObject and it's destructor rather than SEXP protect/unprotect. The code we were generating was correct however hard to read and reason about for users not familiar with R/Rcpp internals. Previously we held the result in a raw SEXP which we unprotected after leaving a special block created for the RNGScope:

RcppExport SEXP rcpp_hello_world() {
BEGIN_RCPP
    SEXP __sexp_result;
    {
        Rcpp::RNGScope __rngScope;
        List __result = rcpp_hello_world();
        PROTECT(__sexp_result = Rcpp::wrap(__result));
    }
    UNPROTECT(1);
    return __sexp_result;
END_RCPP
}

Now, we simply construct an RObject prior to the RNGScope on the stack and then return that object (this ensures that any GC triggered by PutRNGState won't collect the result SEXP before it can be returned):

RcppExport SEXP rcpp_hello_world() {
BEGIN_RCPP
    Rcpp::RObject __result;
    Rcpp::RNGScope __rngScope;
    __result = Rcpp::wrap(rcpp_hello_world());
    return __result;
END_RCPP
}

Use RObject and it's destructor rather than SEXP protect/unprotect.
@jjallaire
Copy link
Member Author

FYI rebuilt the entire Rcpp Gallery with this change and all is well (probably also worth a revdep check). I think this is a straightforward change however I could use a sanity check to make sure I'm not missing something (as the code being modified was put in there originally to avoid a crash under extreme GC conditions and I'd certainly hate to regress back to that behavior). @eddelbuettel @kevinushey @romainfrancois

@kevinushey
Copy link
Contributor

FWIW, we can run unit tests with gctorture(TRUE) on with:

Rcpp:::test(gctorture = TRUE)

so if you feel like heating up your computer a bit, try R --vanilla --slave -e "library(RUnit); Rcpp:::test(gctorture = TRUE)". (I'm running that now and will report if anything is amiss)

@kevinushey
Copy link
Contributor

@eddelbuettel, running a plain Rcpp:::test() fails now because we no longer require(RUnit); instead we just requireNamespace() it and don't qualify our calls to e.g. checkEquals. Should we revert back to just loading RUnit there?

@eddelbuettel
Copy link
Member

@kevinushey : that would make a Suggests: a Depends. Can't we just switch to requireNamespace("RUnit") and do other cleanups as needed?

@jjallaire
Copy link
Member Author

Ran all of the Rcpp tests with gctorture = TRUE and all looks good.

jjallaire added a commit that referenced this pull request Feb 4, 2015
Simplify generated attributes code for RNGScope
@jjallaire jjallaire merged commit c87d7db into master Feb 4, 2015
@jjallaire jjallaire deleted the feature/rngscope-codegen branch February 4, 2015 12:20
jmp75 pushed a commit to jmp75/Rcpp that referenced this pull request Mar 16, 2015
Simplify generated attributes code for RNGScope
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