Skip to content

Passing reference arguments to trampoline methods #2915

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Mar 23, 2021

This is a set of new unit tests validating the correct passing of arguments from Python to C++ via trampoline methods,
i.e. Python-overridden virtual methods of C++ classes. While arguments are correctly passed by value or via pointer (copying vs. reference passing it as expected), there is one exception, namely (lvalue) reference arguments: These are actually copied.
The reason is that the (fixed) policy for this type of action is automatic_reference:

template <return_value_policy policy = return_value_policy::automatic_reference, typename... Args>
object operator()(Args &&...args) const;

which resolves to copy for references:

static handle cast(const itype &src, return_value_policy policy, handle parent) {
if (policy == return_value_policy::automatic || policy == return_value_policy::automatic_reference)
policy = return_value_policy::copy;

However, being able to pass by reference is essential in this case to allow object modifications
performed in Python code to become visible in C++ as well.
The object passed to Python will be alive during the whole lifetime of the method call. Thus it should be safe to pass by reference always. Of course, the Python code shouldn't keep a global copy of the passed variable, because it might/will get invalidated later.

@rhaschke
Copy link
Contributor Author

There is a simple fix to the illustrated issue: Just enforce return_value_policy::reference when calling from a trampoline.

@rhaschke rhaschke force-pushed the virtual-method-reference-args branch from 53e96a8 to a88dc5c Compare March 23, 2021 08:16
@rhaschke

This comment has been minimized.

@rhaschke

This comment has been minimized.

@rhaschke rhaschke marked this pull request as ready for review March 23, 2021 10:29
…n methods

Add tests to ensure that reference arguments passed to Python-overriden methods
(from C++ -> Python -> C++) are actually passed by reference (and not copied),
which is the default return_value_policy for passing from C++ to Python.
@rhaschke rhaschke force-pushed the virtual-method-reference-args branch from 08823ad to fed0a44 Compare March 24, 2021 00:45
@rhaschke rhaschke marked this pull request as draft March 26, 2021 17:09
@rhaschke rhaschke force-pushed the virtual-method-reference-args branch from 940cb36 to 7f598e4 Compare March 29, 2021 14:36
…ence

Otherwise, modifications applied by Python-coded method overrides
would be applied to copies, even though the parameters were passed
by pointer or reference.
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