-
-
Notifications
You must be signed in to change notification settings - Fork 219
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/add more shields #1011
Conversation
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)))); |
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 out Rf_lang4()
from this call and protecting that as well, since IIUC that will be an unprotected R object being passed into Rf_lang2()
, and so in theory could be cleaned up by the GC when the pairlist created in Rf_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 the lang*
helper functions protects the arguments passed in, before the final call is created:
So this pattern should in fact be safe.
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!
For completeness, the reverse dependsn check results are at RcppCore/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