Skip to content

Fix argument passing from trampoline methods #2911

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

Closed
wants to merge 6 commits into from

Conversation

rhaschke
Copy link
Contributor

This adds some more smart_holder tests that mimic some of my (currently failing) application use cases.
Particularly, they consider roundtrip situations where an object is passed around as a reference:

  • Python -> C++ -> Python
  • C++ -> Python -> C++

The single failing test in test_unique_ptr_cref_consumer_roundtrip and test_unique_ptr_cref_roundtrip aren't important. I added them just for completeness. However, test_unique_ptr_consumer_roundtrip is. For this latter test, I didn't look into the reason yet. Will continue tomorrow...

... which shouldn't copy or move the argument
@rhaschke
Copy link
Contributor Author

rhaschke commented Mar 21, 2021

As expected, the unit test was failing because args were passed by-value and not by-reference. However, it wasn't possible to just change the policy to reference or reference_internal.
reference gave errors, because const unique_ptr& only accepts reference_internal.
reference_internal gave errors, because the parent required for keep_alive is null.
Hence, I decided to introduce a new policy to explicitly indicate passing from from Python to C++ via an overridden method. That's the cleanest solution to correctly react to the special needs of this use case.

@rhaschke rhaschke force-pushed the smart_holder branch 3 times, most recently from 332f12b to 5cc4b44 Compare March 21, 2021 05:05
@rhaschke rhaschke marked this pull request as ready for review March 21, 2021 10:21
@rhaschke rhaschke force-pushed the smart_holder branch 2 times, most recently from 1638897 to aefe2c0 Compare March 21, 2021 13:00
@rhaschke rhaschke marked this pull request as draft March 21, 2021 19:24
@rhaschke rhaschke force-pushed the smart_holder branch 2 times, most recently from 4e0b350 to 00db8eb Compare March 21, 2021 20:43
Probably the test is failing, because it passes the arguments by value
instead of by reference.
As expected, the unit test was failing because args were passed by-value
and not by reference. However, it wasn't possible to just change the policy
to reference or reference_internal.
reference gave errors, because const unique_ptr& only accepts reference_internal.
reference_internal gives an error, because the parent required for keep_alive is null.
Hence, I decided to introduce a new policy to explicitly indicate passing from
from Python to C++ via an overridden method. That's the cleanest solution to correctly
react to the special needs of this use case.
@rhaschke rhaschke changed the title Smart holder Fix roundtrip argument passing Mar 21, 2021
@rhaschke rhaschke changed the title Fix roundtrip argument passing Fix argument passing from trampoline methods Mar 21, 2021
@rhaschke
Copy link
Contributor Author

This PR is failing the unit tests, but only with the valgrind config. I have no idea why. The first failing commit is 4b0b8ae.
@YannickJadoul, did you originally introduce the valgrind config? Do you have any idea, why test_gil_scoped.py fails in this situation? Is valgrind too slow?

@rhaschke
Copy link
Contributor Author

Superseeded by #2916.

@rhaschke rhaschke closed this Mar 23, 2021
@EricCousineau-TRI EricCousineau-TRI added the smart holder See: https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst label Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
smart holder See: https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants