[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.
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.
There was a problem hiding this comment.
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.
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.
| void initializePtr() { | ||
| valuePtr = new Value(); | ||
| } | ||
| Value* getNewPtr() const { |
There was a problem hiding this comment.
I see this function moved and became () const { - was that just a cleanup or is it necessary?
There was a problem hiding this comment.
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.
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.
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.
| 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.
Same question here. (edit: or rather, another question about new const-ness)
All the return polices now also work with
propertybindings. 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