Uh oh!
There was an error while loading. Please reload this page.
Propagate py::multiple_inheritance to all children - #3650
Conversation
- Makes py::multiple_inheritance only needed in base classes hidden from pybind11
| // 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:
| // a simple type can never have non-simple ancestors | |
| assert(!tinfo->simple_type||tinfo->simple_ancestors)); |
rwgk
commented
Jan 26, 2022
@jagerman for comment, who I believe was the main author of the multiple inheritance code. |
rwgk
commented
Jan 26, 2022
I'll try to get this through the global testing asap (hopefully the 12:00 batch, in ~2 hours). |
rwgk
commented
Jan 26, 2022
This PR is now in the global testing pipeline (12:00 batch), after first testing manually with ASAN. |
rwgk
left a comment
There was a problem hiding this comment.
The global testing went fine. Results are a bit noisy again, but after looking through I'm convinced we're good to go ahead.
rwgk
commented
Jan 27, 2022
This is also on the smart_holder branch now. Thanks again! |
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