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.

Sign up for GitHub

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

GH-44924: [R] Remove usage of cpp11's HAS_UNWIND_PROTECT #45261

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

GH-44924: [R] Remove usage of cpp11's HAS_UNWIND_PROTECT #45261

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f250c6
remove macro
assignUser Jan 14, 2025
7861bab
update header comment
assignUser Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions r/src/safe-call-into-r-impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,7 @@ bool SetEnableSignalStopSource(bool enabled) {

// [[arrow::export]]
bool CanRunWithCapturedR() {
#if defined(HAS_UNWIND_PROTECT)
return MainRThread::GetInstance().Executor() == nullptr;
#else
return false;
#endif
Comment on lines 48 to -53
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this function at all anymore? Or another way: does MainRThread::GetInstance().Executor() == nullptr ever return false?

Copy link
Member Author

Choose a reason for hiding this comment

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

🤷 I have no idea tbh. Just following the prompt from the cpp11 author ^^

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

cc @romainfrancois

}

// [[arrow::export]]
Expand Down
Loading