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

Merged
merged 1 commit into from
Nov 28, 2024

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.

@wjakob
Copy link
Owner

wjakob commented Nov 28, 2024

This is change is technically a SemVer violation because it changes the behavior of the library. But I can't imagine a case where the former behavior was wanted because it makes the bindings behave quite differently from the original C++ code. So I will merge it into the next minor version unless there are objections.

@wjakob wjakob merged commit f33465c into wjakob:master Nov 28, 2024
31 checks passed
@wjakob
Copy link
Owner

wjakob commented Nov 28, 2024

(Thanks for catching this!)

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.

2 participants