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

Extract Rcpp::unwindProtect() from Rcpp::Rcpp_fast_eval() #873

Merged
merged 5 commits into from Jun 22, 2018

Conversation

Projects
None yet
5 participants
@lionel-
Copy link
Contributor

lionel- commented Jun 21, 2018

This extracts the protecting logic from Rcpp_fast_eval into a new API function Rcpp::unwindProtect(). It is meant for calling any C function that might longjump, for example a function from R's C API.

When compiling against C++11, a std::function overload allows to use function objects or lambdas:

Rcpp::protectUnwind(() [] { return Rf_foo() })

For C++98 a C-style callback interface is provided:

SEXP do(void* data) {
    return Rf_foo();
};

Rcpp::protectUnwind(&do, &data)

I have been thinking we should remove the Rcpp_ prefix from Rcpp_fast_eval(). In fact I think it should become Rcpp::eval(). This makes even more sense since we have switched the .eval() methods of classes (such as Language::eval()) to be synonymous with .fast_eval() methods. The latter should likely be deprecated in the long term. Any objection to rename to Rcpp::eval()?

Also given the API is becoming more general and no longer focused on R evaluation only (and a Rcpp::tryCatch() function is coming up), I think we should rename the define RCPP_PROTECTED_EVAL to RCPP_USE_UNWIND_PROTECT.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 21, 2018

It is meant for calling any C function that might longjump, for example a function from R's C API.

Just this week I was wondering / wishing for something like this. Nice. Will rev.dep check once I get home.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 21, 2018

Also no real objections to the renaming, but I might just suggest to phase it in. Eg when I made the (by the years overdue) update to the Date and Datetime vectors, I offered the new ones (as we have now) plus an opt-in #define for twelve months, and only after twelve months flipped to the new ones.

Something like that may be prudent here.

@lionel-

This comment has been minimized.

Copy link
Contributor Author

lionel- commented Jun 21, 2018

But none of the symbols I'm proposing to rename have been released to CRAN?

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 21, 2018

But we are changing the API -- eval() exists. That is exactly the same as replace the (renamed to old-prefix) Date(time) classes with new-prefix ones, no?

@lionel-

This comment has been minimized.

Copy link
Contributor Author

lionel- commented Jun 21, 2018

I think we only have Rcpp::Rcpp_eval(), so Rcpp::eval() is free to use.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 21, 2018

Maybe. Well then my proposal would be to also rename Rcpp_eval() in due course as a Rcpp_* prefix inside the Rcpp namespace is a little weird :)

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 21, 2018

Codecov Report

Merging #873 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #873   +/-   ##
=======================================
  Coverage   90.09%   90.09%           
=======================================
  Files          71       71           
  Lines        3271     3271           
=======================================
  Hits         2947     2947           
  Misses        324      324
Impacted Files Coverage Δ
inst/include/Rcpp/api/meat/Rcpp_eval.h 66.66% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97ccaeb...2a8e72b. Read the comment docs.

@lionel-

This comment has been minimized.

Copy link
Contributor Author

lionel- commented Jun 21, 2018

Agreed, but then let's rename Rcpp_fast_eval() to eval() before the release to CRAN to minimise API changes.

@kevinushey

This comment has been minimized.

Copy link
Contributor

kevinushey commented Jun 21, 2018

One risk is that (for better or worse) we already encourage the use of using namespace Rcpp; in some contexts which implies adding a definition to eval() to that namespace could risk collisions with some user-existing definitions of eval().

@lionel-

This comment has been minimized.

Copy link
Contributor Author

lionel- commented Jun 21, 2018

The C++11 interface with std::function is now making me doubt. An exception might be thrown from there over the C stack. Will ponder.

@lionel-

This comment has been minimized.

Copy link
Contributor Author

lionel- commented Jun 21, 2018

The C++11 interface with std::function is now making me doubt. An exception might be thrown from there over the C stack. Will ponder.

I think this is fine. When we document this API, we should make it clear that these callbacks cannot throw and are implicitly noexcept (it looks like in C++17 we can make this explicit in the std::function signature). They also can't allocate or depend on destructors, since they are at risk of a longjump.

In the case of a stateful function object, all allocations should be made on the safe side of the C++ stack, i.e. before calling unwindProtect() (typically in the constructor). The callback should then do minimal work and call into C functions as soon as possible.

The situation is similar to destructors which should not throw as well.

Edit: Bottom line this is mostly provided for syntactic purposes, so you can call C functions from a simple C++11 lambda.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 21, 2018

One risk is that (for better or worse) we already encourage the use of using namespace Rcpp;

Do we? I tend to warn against flattened namespaces when I teach Rcpp. Do we make a recommendation somewhere?

(That said, your point is of course very valid. We should not clash.)

@lionel-

This comment has been minimized.

Copy link
Contributor Author

lionel- commented Jun 21, 2018

Should I send a renaming PR from which we could assess name clashes in revdeps?

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 21, 2018

Why don't we stage things? Test (and possibly merge) this, then move on to the next.

The bigger problem, as with the on-list discussion of the gentleman bitten by the RNG scoping, is the code we do not see. Something like eval() clashing is really ... untestable in a way. Bigger packages like V8, reticulate, ... will give us an idea, but it is still just an idea.

@lionel-

This comment has been minimized.

Copy link
Contributor Author

lionel- commented Jun 21, 2018

Why don't we stage things?

Oh yes, I wasn't planning to send one right away.

Something like eval() clashing is really ... untestable in a way.

I see, at first I thought it would produce duplicate symbol errors but of course it wouldn't because these eval() functions would be in different namespaces. Perhaps there's a compiler option that could help detecting ambiguous calls.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 21, 2018

Sorry, I was imprecise. If indeed user A has both eval() functions we see the error. I just think that this may be less likely on our "test coverage code" ie CRAN.

@lionel-

This comment has been minimized.

Copy link
Contributor Author

lionel- commented Jun 22, 2018

@eddelbuettel I forgot to mention that the revdeps should be built with RCPP_PROTECTED_EVAL defined in order to check these changes.

Edit: Let me know when revdeps are finished, I'll rebase on master for linear git history (I forgot to pull upstream before creating this branch).

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 22, 2018

Dang, of course. Thanks for the reminder, @lionel- -- correcting and restarting.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 22, 2018

PR went fine, log committed, merging.

@eddelbuettel eddelbuettel merged commit 995f17b into RcppCore:master Jun 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Enchufa2

This comment has been minimized.

Copy link
Contributor

Enchufa2 commented Jun 29, 2018

I think we should rename the define RCPP_PROTECTED_EVAL to RCPP_USE_UNWIND_PROTECT.

I was planning to release with this define already in the Makevars. Is there any hard decision on this?

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 29, 2018

We cannot do that as it breaks a few key packages (reticulate, v8) and also affects tidyxl.

So for now this has to be per-user and local.

(Also if we set it for good we would set it in a header file.)

@Enchufa2

This comment has been minimized.

Copy link
Contributor

Enchufa2 commented Jun 29, 2018

I mean, I was planning to release my package with this enabled by default, because it works like a charm. But if you plan to change the final name of the define, I should wait.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 29, 2018

I am sorry @Enchufa2 I missed the boat here: Yes, that is absolutely fine, and will give your package user the speed increase. I think this version "should" hit CRAN next month, I can roll one up for the Rcpp drat if you need it now.

@Enchufa2

This comment has been minimized.

Copy link
Contributor

Enchufa2 commented Jun 29, 2018

Great! There is no rush, my release should hit CRAN next month too. Thanks.

@lionel- lionel- deleted the lionel-:impl-unwind-protect branch Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.