Propagate py::multiple_inheritance to all children #3650
Merged
Conversation
This file contains 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
- Makes py::multiple_inheritance only needed in base classes hidden from pybind11
Skylion007
reviewed
Jan 26, 2022
| } | ||
| else if (rec.bases.size() == 1) { | ||
| auto parent_tinfo = get_type_info((PyTypeObject *) rec.bases[0].ptr()); | ||
| tinfo->simple_ancestors = parent_tinfo->simple_ancestors; | ||
| // a child of a non-simple type can never be a simple type | ||
| tinfo->simple_type = parent_tinfo->simple_type; | ||
| } | ||
|
|
There was a problem hiding this comment.
According to the documentation and our testing for the TypeInfo type, a type with non simple_ancestors can never be a simple type. We should add an assert somewhere that verifies this:
Suggested change
| // a simple type can never have non-simple ancestors | |
| assert(!tinfo->simple_type || tinfo->simple_ancestors)); |
Skylion007
approved these changes
Jan 26, 2022
henryiii
approved these changes
Jan 26, 2022
|
@jagerman for comment, who I believe was the main author of the multiple inheritance code. |
|
I'll try to get this through the global testing asap (hopefully the 12:00 batch, in ~2 hours). |
|
This PR is now in the global testing pipeline (12:00 batch), after first testing manually with ASAN. |
rwgk
approved these changes
Jan 27, 2022
|
This is also on the smart_holder branch now. Thanks again! |
Skylion007
added a commit
to Skylion007/pybind11
that referenced
this pull request
Jan 28, 2022
Skylion007
added a commit
that referenced
this pull request
Jan 31, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This builds on the tests split from #3665 into #3649, which should probably be merged first.
This started with a comment that @Skylion007 made on my tests for #3635, and I realized the fix was straightforward. Previously, you had to mark any child of a class that participated in multiple inheritance with
py::multiple_inheritance, even if the child derived from a single base. Now, pybind11 can autodetect this in most cases.I'm not confident this is quite the right fix. It's not clear to me what the difference between
simple_typeandsimple_ancestorsare, and I wonder if the purpose ofsimple_typehas evolved over time as it's only used in one place.A different fix might be to change type_caster_base's simple_type check to simple_ancestors, but then simple type wouldn't be used anywhere?
... actually, I just realized this change makes simple_type and simple_ancestors identical? Simple ancestors is only used for registering/deregistering instances.
It's late here, I'll sleep on it. There must be a subtle side effect of simple_type that I'm just missing and that the tests don't currently cover.
Suggested changelog entry:
* ``py::multiple_inheritance`` is now only needed when C++ bases are hidden from pybind11