Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork219
Feature/add more shields#1011
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-io commentedNov 2, 2019
Codecov Report
@@ Coverage Diff @@## master #1011 +/- ##======================================= Coverage 82.46% 82.46% ======================================= Files 63 63 Lines 3166 3166 ======================================= Hits 2611 2611 Misses 555 555
Continue to review full report at Codecov.
|
| Rf_lang4(removeSym,Rf_mkString(name.c_str()),Storage::get__(),Rf_ScalarLogical(FALSE )) | ||
| ) ); | ||
| Shield<SEXP>str(Rf_mkString(name.c_str())); | ||
| Shield<SEXP>call(Rf_lang2(internalSym,Rf_lang4(removeSym, str,Storage::get__(),Rf_ScalarLogical(FALSE)))); |
kevinusheyNov 4, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
It may also be worth pulling outRf_lang4() from this call and protecting that as well, since IIUC that will be an unprotected R object being passed intoRf_lang2(), and soin theory could be cleaned up by the GC when the pairlist created inRf_lang2() is allocated..
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.
Hmm, I might be wrong. IIUC, each of thelang* helper functions protects the arguments passed in, before the final call is created:
So this pattern should in fact be safe.
kevinushey left a comment
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.
LGTM!
eddelbuettel commentedNov 4, 2019
For completeness, the reverse dependsn check results are atRcppCore/rcpp-logs@74e007b |
As discussed in#1010, a few more unprotected temporaries were floating around. This attempts to cover a few more.
@kevinushey give it a look when you have a moment