FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add an explicit destructor function argument to registry::push_back() by res2k · Pull Request #170 · boostorg/python · GitHub

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

Open
res2k wants to merge 4 commits into
boostorg:developfrom
res2k:converter-destruct
Open

res2k wants to merge 4 commits into
boostorg:developfrom
res2k:converter-destruct

Conversation

res2k commented Nov 1, 2017

Copy link
Copy Markdown
Contributor

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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL