Uh oh!
There was an error while loading. Please reload this page.
Preserved list refactor - #92
Conversation
9ec6602 to
7a266deComparebkietz
commented
Aug 16, 2020
"testing if installed package can be loaded from temporary location" failed with |
jimhester
commented
Aug 17, 2020
One issue with this approach and part of the reason I didn't mind having multiple protect lists for each compilation unit is currently there is no requirement for the cpp11 package itself to be loaded. An alternative would be to create the protect list in the init function in the generated code. This would give us one protect list per loaded R package that uses cpp11. Line 382 in 07b7ba7 |
bkietz
commented
Aug 17, 2020
With multiple protect lists, cpp11::cpp_function('int release_all() { int n = Rf_length(cpp11::protect_list) - 1; cpp11::release_existing_protections(); return n;}')which wouldn't have the desired effect. |
jimhester
commented
Aug 17, 2020
Yeah, though code needing to manually call Creating the list as an object in the actual |
Rather than an object in the global environment. Using an option is consistent with CRAN policies, whereas it is against policy for a package to modify the global environment
jimhester
commented
Sep 8, 2020
I just thought of a way to do this that will give us a singleton and is not against CRAN policies. Setting package specific global options are allowed by CRAN, so we could store the preserve list there. I did this in 9c5dd15 |
jimhester
commented
Sep 9, 2020
Unfortunately there seems to be something wrong with the implementation, we are getting an error on the CI with R 3.4 (https://github.com/r-lib/cpp11/pull/92/checks?check_run_id=1086545888#step:15:15)
And I see the same error with some of the tests in vroom with this PR. I am still trying to track down the issue. |
Instead of directly, since `options()` and `getOption()` call `duplicate()` on their return, and if we try to duplicate the pairlist we run into errors.
jimhester
commented
Sep 9, 2020
Ok so the issue was due to option values being duplicated by the |
jimhester
commented
Sep 10, 2020
I think this is all working now, @bkietz if you have time to review my changes I can merge this. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
protect_sexp/release_protect/...topreserved.insert/preserved.releaseto highlight the similarity in intent toR_PreserveObjectand decrease the semantic overload of "protect"cpp11::protect_listto an internal in R namedcpp11:::.preserve_list. This allows the list to be a singleton since any shared object we load will look up the samecpp11:::.preserve_list. By contrast a new c++ static variable will be instantiated for each translation unit we compile, resulting in multiple unrelated preserve_lists:cpp11::preserve()sexp& sexp::operator=(const sexp& rhs)did not release its data before acquiring a new preservation token