WIP: Multiple inheritance support #410
Conversation
| @@ -754,7 +794,7 @@ template <typename type, typename holder_type> class type_caster_holder : public | |||
| using type_caster_base<type>::value; | |||
| using type_caster_base<type>::temp; | |||
|
|
|||
| bool load(handle src, bool convert) { | |||
| bool load(handle src, bool convert) {//XXX | |||
There was a problem hiding this comment.
(Multiple inheritance involving shared pointer classes TBD, just needs to be copied from above)
|
@wjakob support for multiple inheritance will certainly be welcome!!! I will be happy to test this on PyRosetta when it is merged (will require some time to implement it though) so please drop me a line when its ready for testing. A few questions:
|
|
|
||
| template <template<typename> class Predicate, typename Child> | ||
| struct add_base_helper<Predicate, Child> { | ||
| static void apply(detail::type_record &) { } |
There was a problem hiding this comment.
I haven't actually compiled the following snippet, so take it with a grain of salt, but it's a possible simplification (non-recursive, non-SFINAE):
template <typename Base, typename Child>
void add_base_helper(type_record &rec, std::true_type /*is_base*/) {
rec.add_base(&typeid(Base), [](void *src) -> void * {
return static_cast<Base *>(reinterpret_cast<Child *>(src));
});
}
template <typename...> void add_base_helper(type_record &, std::false_type /*is_base*/) { }Called as:
add_base_helper<options, type_>(record, is_base_class<options>())...(Plus the usual variadic call {(f(), 0)...} dance.)
There was a problem hiding this comment.
That's much better, thanks.
@lyskov: The goal of posting the WIP was to get other people to test drive this sufficiently before it is merged. Regarding your other questions: I think extending from Python should work just the same, though it would be good for somebody to verify this. A more tricky case would entail classes in Python that perform multiple inheritance from multiple C++ or Python classes -- I think that will run into limitations of the current patch set. |
|
rebased and integrated feedback |
wjakob
force-pushed
the
mi
branch
2 times, most recently
from
88807b0
to
c3f7942
Compare
|
@wjakob oh, so its ready for testing - great, i will give it a try then, i will let you know how it goes. |
|
@wjakob not sure if it is possible to catch this during compilation: right now specifying the same base type multiple times lead to cryptic runtime error during runtime: |
|
Also, the same kind of error appears if bases was specified once but there was a virtual inheritance somewhere before... |
|
I am guessing that for cases of |
|
@lyskov: For multiple inheritance, could you post the source code snippet/ |
|
Yes, i already working on minimal example for this... |
|
code example to trigger this error with just multiple bases: #include <pybind11/pybind11.h>
class Common {};
class Base1 : public virtual Common {};
PYBIND11_PLUGIN(example) {
pybind11::module m("example", "pybind11 example plugin");
{
pybind11::class_<Common> cl(m, "Common");
cl.def(pybind11::init<>());
}
{
pybind11::class_<Base1, Common, Common> cl(m, "Base1"); // Deliberately specifying base class twice - this will lead to an error
cl.def(pybind11::init<>());
}
return m.ptr();
} |
|
And a bit more complex example when using virtual inheritance: While this works: #include <pybind11/pybind11.h>
class Common {};
class Base1 : public virtual Common {};
class Base2 : public virtual Common {};
class Derived : public Base1, public Base2, public virtual Common {};
PYBIND11_PLUGIN(example) {
pybind11::module m("example", "pybind11 example plugin");
{
pybind11::class_<Common> cl(m, "Common");
cl.def(pybind11::init<>());
}
{
pybind11::class_<Base1, Common> cl(m, "Base1");
cl.def(pybind11::init<>());
}
{
pybind11::class_<Base2, Common> cl(m, "Base2");
cl.def(pybind11::init<>());
}
{
pybind11::class_<Derived, Base1, Base2, Common> cl(m, "Derived");
cl.def(pybind11::init<>());
}
return m.ptr();
}This does not (i know it rather silly, but right now i could not extract the real one from my code base that trigger the same error): #include <pybind11/pybind11.h>
class Base1 {};
class Base2 : public virtual Base1 {};
class Derived : public virtual Base1, public Base2 {};
PYBIND11_PLUGIN(example) {
pybind11::module m("example", "pybind11 example plugin");
{
pybind11::class_<Base1> cl(m, "Base1");
cl.def(pybind11::init<>());
}
{
pybind11::class_<Base2, Base1> cl(m, "Base2");
cl.def(pybind11::init<>());
}
{
pybind11::class_<Derived, Base1, Base2> cl(m, "Derived");
cl.def(pybind11::init<>());
}
return m.ptr();
}Edit: posted the missing code |
|
Would it be possible to generate more informative error message during import? Because right now it only state either |
|
Yep, the error messages could be a bit more clear, I'll take a stab at that. Note that specifying the same base class multiple times to Regarding virtual inheritance, see the following (in the context of Boost.Python bindings): http://stackoverflow.com/questions/21221628/virtual-inheritance-c-and-boost-python |
|
Re virtual inheritance: Interesting, - do you have clear idea what will work and what will not? Because it does not look like it is the 'diamond case' since the following example is seems to work: #include <pybind11/pybind11.h>
class Common {};
class Base1 : public virtual Common {};
class Base2 : public virtual Common {};
class Derived : public Base1, public Base2 {};
PYBIND11_PLUGIN(example) {
pybind11::module m("example", "pybind11 example plugin");
{
pybind11::class_<Common> cl(m, "Common");
cl.def(pybind11::init<>());
}
{
pybind11::class_<Base1, Common> cl(m, "Base1");
cl.def(pybind11::init<>());
}
{
pybind11::class_<Base2, Common> cl(m, "Base2");
cl.def(pybind11::init<>());
}
{
pybind11::class_<Derived, Base1, Base2> cl(m, "Derived");
cl.def(pybind11::init<>());
}
return m.ptr();
}It would be great if we can formulate formal definition what can and what will not work so i can put it into code for bindings generator. |
|
The error messages are now a bit more verbose: In your previous example, the output is It turns out that Python just does not allow this kind of class hierarchy (try creating without C++ in pure Python). |
|
There is now a bit of documentation which discusses the syntax and limitations |
|
|
||
| .. code-block:: cpp | ||
|
|
||
| py::class_<MyType, BaseType2>(m, "MyType", py::multiple_inheritance()); |
There was a problem hiding this comment.
This would be nicer to add this as a template option annotation, rather than going through the process_attribute mechanism, so that it stays with the base classes, i.e.:
py::class_<MyType, py::multiple_inheritance, BaseType2>(...)That is pretty easy to do: just add a new predicate for the type to the list of predicates at the beginning of class_ like this:
template <typename T> using is_mi_annotation = std::is_same<T, multiple_inheritance>;then update the is_valid_class_option predicate with || is_mi_annotation<T>::value, and finally, add this to the class_ constructor:
if (detail::any_of_t<is_mi, options...>::value) record.multiple_inheritance = true;There was a problem hiding this comment.
Since multiple_inheritance doesn't really parameterize the type of the class (not a base/alias/holder), I would find it consistent to keep the marker in the (non-template) argument list similar to how such flags are used for method definitions. Other opinions?
|
Re: multiple inheritance on the python side, the documentation current says that you can't, and that this is a limitation of Python. That's true, in a sense, but I think there's a potential workaround. While true, it is actually something that seems, in theory, as though it could be worked around, by making pybind11 classes lay-out compatible. I haven't actually tried this, but it seems to me that if every pybind-registered type consisted of, say, just a single pointer (i.e. to the real instance, stored somewhere in pybind11-controlled memory), classes would have a compatible layout, and Python would allow the multiple inheritance. (It might be necessary to have more than one pointer; e.g. a second pointer to type information, but that should still be layout compatible as long as every class has the same thing). That would, of course, require pybind11 to have a layer of pointer redirection: methods would need to go to a (very hot) pybind11 dispatch function that invokes the actual function with the dereferenced pointer, but unless I'm missing something, it seems like a feasible approach. (Of course, I think it's way beyond the scope of the current PR, at least as currently implemented, which is about allowing MI on the C++ side). |
| 2. As was previously discussed in the section on :ref:`overriding_virtuals`, it | ||
| is easy to create Python types that derive from C++ classes. It is even | ||
| possible to make use of multiple inheritance to declare a Python class which | ||
| has e.g. a C++ and a Python class as bases. However, any attempt to create |
There was a problem hiding this comment.
It's a bit more limited than that: you can't have two different C++ classes anywhere in your inheritance tree; e.g. having a C++ and Python classes as bases won't work if the Python class itself depends on a C++ class (or another Python class that depends on a C++ class, ad infinitum).
|
Great catch and nice workaround @dean0x7d! |
|
Update: looks like that didn't fix it. |
|
I've just double checked that it really is an issue specific to old-style classes: this change bef164f results in a clean CI build. Note that
The |
|
This latest patch seems to have fixed it! |
|
Just tried latest version on my project: i am still receiving |
|
This sounds like a fairly bizarre failure (though AFAIK nobody else has run into it). I would be happy to try to track it down if you could turn this into a reproducible test case. |
|
(Also, I assume this happens even without PR #410, right?) |
i was wrong about this. After double checking i am sure that this issue arise only when build with #410 version. Is there is anything i can do to help track this down? Standard methods of isolation is not very useful here since this error happened after ~800+ classes was bound and class_ declaration for it look innocent enough. Is there is way to obtain traceback that would be helpful to debug this? Edit: remove markup for clarity |
|
A backtrace might help here. With a debug build, you could launch Python in a debugger and break on "__cxa_throw" to get a backtrace. |
|
Do i need Python debug build for this as well? |
|
That would help even more since assertions might catch another error before that obscures the real reason. |
|
here backtrace for this, hope this helps! |
|
A bit of extra info: - I can not reproduce this on mac with Python-2.7. So it might be possible that problem is Python-3 specific or only appear on Linux... |
|
That backtrace was indeed helpful (and reminiscent of the old bug #277). I've committed a fix, can you see if it works? |
|
It's great that you have a codebase with so many classes. These kind of GC-in-an-unfortunate-place issues are so tricky to track down with just the default test suite. |
|
Assuming this fixes the issue reported by @lyskov, my plan is to merge this in the next couple of days. Speak now or forever hold your peace (or something like that ;)). |
|
Glad to hear that backtrace was helpful! I can confirm that c5c8b96 is indeed fixed the issue! I am now in process of running rest of my tests and will post here when its done (should take ~24hr or less) |
|
Just letting you know that I am finished testing this PR with my codebase and so far have not found any more issues, - I am looking forward to see this code in master! |
|
Great, good to hear! Squashed into a smaller number of commits. |
This is an early snapshot of a potential way to support multiple inheritance in a minimally intrusive way. Standard single inheritance is supported using a fast fallback that matches the current implementation.
Somewhat surprisingly, the change and related simplifications actually reduce the size of generated binaries by 4.4% :).
There are probably a ton of cases that I didn't think of, so this is up for public review now.