Skip to content

Conversation

@rhaschke
Copy link
Contributor

This fixes #1694. I wanted to contribute something after filing issues #2046 and #2047.
The fix just adds perfect forwarding for method args to support both, lvalue and rvalue arguments.
The unit test required a minor adaptation as ExampleMandA::add1() now copies and moves (instead of copying twice).

@wjakob
Copy link
Member

wjakob commented Dec 31, 2019

Looks like a simple enough change. I'm curious though what kinds of types will actually be affected by this? std::string? What about STL types like std::vector<..>? Does this commit by itself lead to any changed behavior wrt. custom types bound using py::class_<>?

@wjakob
Copy link
Member

wjakob commented Dec 31, 2019

This is a smaller component of #2047, correct?

@rhaschke
Copy link
Contributor Author

rhaschke commented Dec 31, 2019

Yes, #2047 building on top of this one.

@rhaschke
Copy link
Contributor Author

I'm curious what kinds of types will actually be affected by this?

This PR just enables perfect forwarding. It's not related to particular types, but to the type specifiers (e.g. & or &&) in method arguments.

Does this commit by itself lead to any changed behavior wrt. custom types bound using py::class_<>?

No, the PR just enables rvalue arguments in methods. If the custom type supports rvalue reference casting (only with #2048), it will be possible to forward them to such method arguments.

@bstaletic
Copy link
Collaborator

bstaletic commented Jul 7, 2020

@wjakob For example, this would fix #1694

Full disclosure: I haven't actually tested this pull request.

@wjakob
Copy link
Member

wjakob commented Jul 7, 2020

Seems quite safe, let's merge it and see if anyone complains.

@wjakob wjakob merged commit f2226ae into pybind:master Jul 7, 2020
@rhaschke
Copy link
Contributor Author

rhaschke commented Jul 7, 2020

Great to see some progress here. Thanks for merging.

@rhaschke rhaschke deleted the fix-1694 branch July 7, 2020 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rvalue reference parameters work in constructors, but not in other functions

4 participants