-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
[embind] Return value policy support for properties. #21935
base: main
Are you sure you want to change the base?
[embind] Return value policy support for properties. #21935
Conversation
All the return polices now also work with `property` bindings. Using `return_value_policy::reference()` is especially helpful for binding child objects of a class so they don't always create copies and potentially leak. Fixes emscripten-core#6402, emscripten-core#17573
// Getter and setter. | ||
.property("getterSetterValue", &ValueHolder::get, &ValueHolder::set, return_value_policy::reference()) | ||
// std::function getter. | ||
.property("getterPtrWithStdFunctionReference", std::function<const Value*(const ValueHolder&)>(&value_holder), return_value_policy::reference()) | ||
; |
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 think the use case I had in mind for this feature was with raw pointers - is that supported as well? If so can it be tested here?
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.
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.
Thanks, I see.
Though I was expecting to read allow_raw_pointers()
. Is that not needed in these cases? I can't tell from the docs.
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'm hoping to deprecate allow_raw_pointers()
in favor of return policies. Using a return value policy implies that raw pointers are allowed. There's a little note on this here. I should probably update that to be more generic now that RVP is allowed on properties 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.
I see, thanks!
void initializePtr() { | ||
valuePtr = new Value(); | ||
} | ||
Value* getNewPtr() const { |
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 see this function moved and became () const {
- was that just a cleanup or is it necessary?
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.
Embind requires getter function's to be const. We could have a const and non-const version for testing functions/properties, but doesn't really seem worth it.
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.
Makes sense. Is that a new change with this PR, though? That's what I'm trying to understand, as the function seems to have worked ok as non-const before.
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.
const
is not a new requirement for getters. I added the const
here since I now use value_holder
to also test a getter property. value_holder
calls ValueHolder.getNewPtr
so it must also be const.
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 see, thanks.
Value* value_holder(ValueHolder& target) { | ||
return target.getNewPtr(); | ||
const Value* value_holder(const ValueHolder& target) { | ||
return target.getNewPtrConst(); |
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.
Same question here. (edit: or rather, another question about new const-ness)
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 can't say I have a full understanding of the C++ template magic but it looks reasonable. Docs and tests look good.
All the return polices now also work with
property
bindings. Usingreturn_value_policy::reference()
is especially helpful for binding child objects of a class so they don't always create copies and potentially leak.Fixes #6402, #17573