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

Add an explicit destructor function argument to registry::push_back() #170

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

res2k
Copy link
Contributor

@res2k res2k commented Nov 1, 2017

This patch adds an explicit 'destruct' function argument to registry::push_back().

Motivation:
See the 'opaque_ref' test in commit 834f0d1. This creates a wrapper for a C++ type where only the declaration is known. This may happen if you want to wrap functions/methods which take a const ref to a certain type as an argument, but instances of that type are produced elsewhere (perhaps a different Python module).
As it is now the 'opaque_ref' test fails because the associated Python module references ~Opaque() - which is undefined. This is also counterintuitive, since Opaque is actually not constructed anywhere!
The culprit is rvalue_from_python_data<T>::~rvalue_from_python_data() which unconditionally references the dtor of T. If we replace this dtor call with an explicit destructor function we only need a dtor reference if we're actually creating an instance.

Source compatibility?:
Adding a parameter to registry::push_back() may break existing sources. It's not clear to me whether it's supposed to be a "stable" API or whether breakage is acceptable. If it's the former I'd look into creating a source-compatible variant.

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