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

Remove cpp11_should_unwind_protect altogether #327

Merged

Conversation

DavisVaughan
Copy link
Member

@DavisVaughan DavisVaughan commented Jul 26, 2023

Closes #325
Closes #326

  • Update documentation to talk about dangers of nested unwind protection
  • Update documentation to talk about dangers of using C++ code that calls destructors from within a manual unwind_protect() call (since the destructors won't run if a longjmp occurs)
  • Update documentation to talk about performance of cpp11 character vectors in a loop, and how to use unwind_protect() manually to avoid the performance hit
  • Update NEWS bullet
  • 2nd order revdep checking (these came back clean)

Note that this makes the example from #298 very slow again, because nested unwind_protect() is no longer "optimized". This is an example where character vector manipulation may be better with a manual unwind_protect() + R C API usage

@DavisVaughan DavisVaughan force-pushed the fix/faulty-unwind-protect-variable branch from 5525adc to 98343fe Compare July 27, 2023 19:25
@DavisVaughan DavisVaughan force-pushed the fix/faulty-unwind-protect-variable branch from 98343fe to 7fd15aa Compare July 27, 2023 19:27
@DavisVaughan DavisVaughan marked this pull request as ready for review July 27, 2023 21:14
Copy link
Contributor

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@lionel- lionel- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice writeup!

vignettes/FAQ.Rmd Outdated Show resolved Hide resolved
vignettes/FAQ.Rmd Outdated Show resolved Hide resolved
vignettes/FAQ.Rmd Outdated Show resolved Hide resolved
@DavisVaughan DavisVaughan merged commit 4616f1c into r-lib:main Jul 28, 2023
14 checks passed
@DavisVaughan DavisVaughan deleted the fix/faulty-unwind-protect-variable branch July 28, 2023 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants