GH-44924: [R] Remove usage of cpp11's HAS_UNWIND_PROTECT #45261
Conversation
| bool CanRunWithCapturedR() { | ||
| #if defined(HAS_UNWIND_PROTECT) | ||
| return MainRThread::GetInstance().Executor() == nullptr; | ||
| #else | ||
| return false; | ||
| #endif |
There was a problem hiding this comment.
Do we need this function at all anymore? Or another way: does MainRThread::GetInstance().Executor() == nullptr ever return false?
There was a problem hiding this comment.
🤷 I have no idea tbh. Just following the prompt from the cpp11 author ^^
There was a problem hiding this comment.
ah, the header explains it, I'll update the comment but from how I read it we still need to check for the executor
// Unwind protection was added in R 3.5 and some calls here use it
// and crash R in older versions (ARROW-16201). Implementation provided
// in safe-call-into-r-impl.cpp so that we can skip some tests
// when this feature is not provided. This also checks that there
// is not already an event loop registered (via MainRThread::Executor()),
// because only one of these can exist at any given time.
github-actions
bot
added
awaiting changes
github-actions
bot
added
awaiting change review
github-actions
bot
added
awaiting merge
### Rationale for this change Cleanup a minor formatting issue introduced in #45261 ### What changes are included in this PR? Remove two new lines ### Are these changes tested? Yes operative changes, linter should pass ### Are there any user-facing changes? No Authored-by: Jonathan Keane <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 7167ed1. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…e#45261) ### Rationale for this change The macro is no longer required on R >= 4.0 which is our minimum version. ### What changes are included in this PR? Remove use of HAS_UNWIND_PROTECT ### Are these changes tested? ci ### Are there any user-facing changes? no * GitHub Issue: apache#44924 Authored-by: Jacob Wujciak-Jens <[email protected]> Signed-off-by: Jacob Wujciak-Jens <[email protected]>
### Rationale for this change Cleanup a minor formatting issue introduced in apache#45261 ### What changes are included in this PR? Remove two new lines ### Are these changes tested? Yes operative changes, linter should pass ### Are there any user-facing changes? No Authored-by: Jonathan Keane <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
Rationale for this change
The macro is no longer required on R >= 4.0 which is our minimum version.
What changes are included in this PR?
Remove use of HAS_UNWIND_PROTECT
Are these changes tested?
ci
Are there any user-facing changes?
no
HAS_UNWIND_PROTECT#44924