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.

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

Default to rv_policy::move when binding in-place operators #803

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oremanj
Copy link
Contributor

@oremanj oremanj commented Nov 27, 2024

The idiomatic return type for T::operator+= in C++ is T&, because the idiomatic return value is *this. When binding nb::self += nb::self, or any other augmented-assignment operator, for a class that follows this convention, nanobind currently applies rv_policy::automatic to the T& return value and makes a copy of the referenced T. I think this is counter to users' expectations: a C++ type that defines these augmented assignment operators is mutable, so it should behave like mutable objects do in Python, and let += preserve object identity. The problem is demonstrated by the test in this PR, which fails on current master.

Fix this issue by defaulting to rv_policy::move for the return value of augmented assignment operators. This will work as it does today if the operator returns by value, but be able to reuse the same Python object if the operator returns *this by reference. Users can still override the default by passing an rvp as an additional argument to .def().

Footnote: This issue suggests a potential wider problem when binding "generative methods" like Foo& Foo::withBar(int bar) { myBar = bar; return *this; }; using those in Python will make a copy too, because rv_policy::copy prevents reusing an existing pyobject on nanobind. (This is a difference from pybind and maybe should be mentioned in the porting section?) However, I don't think we can conclude that all methods returning reference-to-self-type return *this; it's a much more robust assumption for augmented assignment operators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant