-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
R: Fix protection errors for relational API #6469
Conversation
c8ebf68
to
9fc219d
Compare
} | ||
|
||
[[cpp11::register]] SEXP rapi_rel_set_diff(duckdb::rel_extptr_t rel_a, duckdb::rel_extptr_t rel_b) { | ||
auto res = std::make_shared<SetOpRelation>(rel_a->rel, rel_b->rel, SetOperationType::EXCEPT); | ||
return make_external<RelationWrapper>("duckdb_relation", res); | ||
|
||
cpp11::writable::list prot = {rel_a, rel_b}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but can't we do the list wrapping in make_external_prot, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean passing two arguments to make_external_prot()
, or just inlining the prot
variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of the latter but not sure if it still shows the desired behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect it would, the temporary list
won't be destroyed for the duration of the make_external()
call, and hece will remain protected. We can try in a second iteration.
Closes #6465.
This adds a tiny change to the vendored cpp11 code, we should eventually upstream it to take advantage of future updates.