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

Control whether exceptions include the call and C++ stack with a macros #868

Merged
merged 5 commits into from Jun 15, 2018

Conversation

Projects
None yet
3 participants
@romainfrancois
Copy link
Contributor

romainfrancois commented Jun 11, 2018

Follow up to #663.

The idea is to have a general default for whether exceptions generated by Rcpp include the call stack or not.

#define RCPP_DEFAULT_INCLUDE_CALL false
#include <Rcpp.h>

// [[Rcpp::export]]
void Rcpp_exception(){
    throw Rcpp::exception("ouch");
}

// [[Rcpp::export]]
void eval_error(){
    Rcpp_eval(Rf_lang1(Rf_install("ouch")), R_EmptyEnv);
}

Adding the RCPP_DEFAULT_INCLUDE_CALL which by default is defined to true in Rcpp. When it is defined to false such as above, the default for include_call in Rcpp::exception is set to false, and the other exceptions, e.g. eval_error don't include the call and c++ stack.

> Rcpp::sourceCpp('inst/unitTests/cpp/Exceptions_nocall.cpp')
> Rcpp_exception()
Error: ouch
> eval_error_no_call()
Error: Evaluation error: could not find function "ouch".
> stop_no_call()
Error: ouch

Having this in place means that a package can globally decide not to include the call stack for all exceptions that are generated by Rcpp internal code, and also still be able to use stop(...) instead of throw Rcpp::exception(tfm::format(...).c_str(), include_call = false) which is I believe the only way available after #663.

The alternative to this PR would be to rework all other exception classes so that they also have the bool include_call setting.

This is not necessary as these other exceptions are mostly thrown by Rcpp itself and so there is no opportunity to set the include_call argument in their constructors.

romainfrancois added some commits Jun 11, 2018

Also use RCPP_DEFAULT_INCLUDE_CALL for exception classes other than R…
…cpp::exception

use Shelter instead of Shield because e.g. the result of get_last_call() would not be protected long enough.

rework exception_to_r_condition and rcpp_exception_to_r_condition so that they share the same code via a template
Additional tests for Rcpp::exception and eval_error. Trsting that the…
…y don't include call and cppstack when RCPP_DEFAULT_INCLUDE_CALL is defined to false
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 11, 2018

Codecov Report

Merging #868 into master will decrease coverage by 0.22%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #868      +/-   ##
==========================================
- Coverage   90.21%   89.99%   -0.23%     
==========================================
  Files          70       71       +1     
  Lines        3261     3269       +8     
==========================================
  Hits         2942     2942              
- Misses        319      327       +8
Impacted Files Coverage Δ
inst/include/Rcpp/exceptions.h 7.69% <ø> (ø) ⬆️
inst/include/Rcpp/protection/Shelter.h 0% <0%> (ø)

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 5d5cdb8...4ff9196. Read the comment docs.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 11, 2018

If you could, please consider writing submissions and commit message without emojis.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 11, 2018

Thanks for tieing it into #663. We'll take a look. At some point we may need a full grid documenting how stop() and friends can be called.

@romainfrancois

This comment has been minimized.

Copy link
Contributor Author

romainfrancois commented Jun 11, 2018

if you could, please consider writing submissions and commit message without emojis.

I can do that, maybe this should be part of your contributions guidelines.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 11, 2018

I edited these last week and decided against making it a requirement, but the language I added was milder under Coding Style:

It would be nice if the contributed code followed existing conventions for whitespace and indentation.

I am just asking you (as well as everybody else) rather gently to consider following existing style. Most people have no problem with that.

@romainfrancois

This comment has been minimized.

Copy link
Contributor Author

romainfrancois commented Jun 11, 2018

I don't have a problem with that.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 11, 2018

Excellent.

Now, as we're at style: We have one pending PR I'll likely merge soon. Do you want to rebase then, or would it be ok if I squash-merged this one? I prefer to keep the git graph tidy and narrow.

@romainfrancois

This comment has been minimized.

Copy link
Contributor Author

romainfrancois commented Jun 11, 2018

Feel free to squash.

@eddelbuettel eddelbuettel merged commit 1b33aad into RcppCore:master Jun 15, 2018

1 check passed

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

This comment has been minimized.

Copy link
Member

eddelbuettel commented Jun 15, 2018

Worked like a charm in reverse depends, and now part of Rcpp 0.12.17.3 -- so thanks!

@romainfrancois

This comment has been minimized.

Copy link
Contributor Author

romainfrancois commented Jun 18, 2018

Thanks.

@romainfrancois romainfrancois deleted the romainfrancois:f-default-include-call branch Jun 18, 2018

romainfrancois added a commit to tidyverse/dplyr that referenced this pull request Jun 18, 2018

Depend on Rcpp 0.12.17.3 so that we can benefit from RcppCore/Rcpp#868
…and remove the calls from Rcpp exceptions. closes #3662

romainfrancois added a commit to tidyverse/dplyr that referenced this pull request Aug 6, 2018

Depend on Rcpp 0.12.17.3 so that we can benefit from RcppCore/Rcpp#868
…and remove the calls from Rcpp exceptions. closes #3662
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.