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

Enable unwind protection by default #1225

Merged
merged 6 commits into from
Jul 27, 2022
Merged

Enable unwind protection by default #1225

merged 6 commits into from
Jul 27, 2022

Conversation

Enchufa2
Copy link
Member

@Enchufa2 Enchufa2 commented Jul 23, 2022

As discussed, this PR enables unwind protection by default:

  • RCPP_USE_UNWIND_PROTECT has no effect anymore.
  • Instead, it can be disabled by defining RCPP_NO_UNWIND_PROTECT, similarly to other RCPP_NO_* defines.
  • The associated plugin is deprecated. To avoid errors, it currently shows a warning and has no effect. It could be removed in a future release.

Checklist

  • Code compiles correctly
  • R CMD check still passes all tests
  • Prefereably, new tests were added which fail without the change
  • Document the changes by file in ChangeLog

@Enchufa2
Copy link
Member Author

Enchufa2 commented Jul 23, 2022

Added the following fixes:

  • 666fa8c: ExpressionVector::eval() doesn't work with fast evaluation, because Rf_eval just returns the input expression as is. Therefore, I switched it to using Rcpp_eval() instead, to preserve the usual behaviour.
  • fdadbb4: Exceptions try to parse the output from sys.calls() in order to get_last_call(), taking advantage of the call stack set by Rcpp_eval(). With fast evaluation, this trick doesn't work anymore. So I've commented out the checks for the call name, unless we come up with another trick to get the last call under fast evaluation.

About the deprecation of the plugin, .Deprecated() is handy when a function is deprecated, because it automatically captures the function name and allows you to redirect the user to another one; otherwise, it's just a wrapper around warning(). Given that this is not a function and we would need to override the default behaviour anyway, I just use warning() directly.

Also, a quick search shows only 7 results for this plugin, and one is Rcpp itself. So I could send patches to those 6 packages and there should be no trouble removing the plugin in the future.

@eddelbuettel
Copy link
Member

Catching up after a day in the country-side with a nine-hour power outage :) The plan with respect to patches to six packages sounds good -- you could follow the pattern in #1158 which set up a useful model. Not suggesting we must do that but I felt it helped and provided transparency.

nacnudus added a commit to nacnudus/tidyxl that referenced this pull request Jul 26, 2022
@eddelbuettel
Copy link
Member

Following the usual full reverse-depends checks (discussed with @Enchufa2 in parallel over slack) and the one change needed already merged (thanks to @Enchufa2 for the PR and @nacnudus for the merge) this can be merged. Yay! I will follow up with a minimal cleanup commit to roll the version number.

@eddelbuettel eddelbuettel merged commit 8d30fd2 into master Jul 27, 2022
@Enchufa2
Copy link
Member Author

Yay! Thanks. I'll contact the maintainers of those packages that use the plugin to switch to the define instead, so that one day it can be safely removed. I'll document the process as suggested.

@Enchufa2 Enchufa2 deleted the unwindProtect branch July 27, 2022 11:10
eddelbuettel added a commit that referenced this pull request Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants