Fail to compile if attempting to specifying MI bases via class_ ctor parameters

[jagerman]

Currently we only allow multiple inheritance via class_ template parameters; this extends the support to class_ instances passed via the derived class_ constructor arguments as well.

Edit: see discussion below. We can't support MI via parameters in the most useful case, so produce a compile time error instead.

Fixes #741

[wjakob]

neat! That looks great (including your implementation)

[wjakob]

Looks like MSVC doesn't like your template forward declaration.

[dean0x7d]

As far as I have gathered, the main motivation for the alternative constructor-argument base specification is this: http://pybind11.readthedocs.io/en/latest/advanced/misc.html#partitioning-code-over-multiple-extension-modules

If that's the case, the class_<...> compile-time information is lost and it reverts to adding bases via handle and the original problem comes back. It would be useful to add an error message in that case.

[jagerman]

Ah, right, that's going to be a problem.

[dean0x7d]

If the compile-time type information is always required for multiple inheritance, perhaps it would be enough to require that MI must use the template parameter specification, limit the ctor arguments to single-inheritance-only and just report an error if > 1 handles are passed via ctor.

[jagerman]

Yeah, that sounds reasonable.

[jagerman]

Even better: it should be possible to make it a compile-time error.

[wjakob]

@dean0x7d: I'm not sure I follow -- why is compile-time information needed for MI? The documentation you referenced talks about cyclic dependencies, but that's not the case here, no?

[jagerman]

@wjakob - because MI requires a static_cast<BaseN>(reinterpret_cast<Derived>(voidptr)): unlike single inheritance, we can't reinterpret_cast directly to the BaseN because BaseN might not be at the pointer itself.

[wjakob]

ah right.. I forgot about that. We need to create all the converters that let us jump through the hierarchy.

[jagerman]

So while we could support it for actual class_<Base, ...> instances, it's probably not worth bothering with since we can't support it in the most useful base-class-as-argument case.

[dean0x7d]

It's pretty awesome that this can be a compile-time error!

One minor point, and feel free to ignore this since it's more about code style. The first none_of and the first constexpr_sum are repeating a bit of work. Consider assigning the sum to a variable to reuse the result. In general, assigning to variables would make the code more self-documenting, avoiding the need for any extra comments. E.g.:

constexpr auto compiletime_bases = constexpr_sum(is_pyobject<options>::value...);
constexpr auto runtime_bases     = constexpr_sum(is_pyobject<Extra>::value...);
constexpr auto needs_multiple_inheritance = compiletime_bases + runtime_bases > 1 ||
                                            any_of<std::is_same<multiple_inheritance, Extra>...>();
static_assert(!(needs_multiple_inheritance && runtime_bases > 0),
              "Error: multiple inheritance bases must be specified via class_ template options");

(In addition to the variable names, the error message itself shows the overall intent.)

[jagerman]

There's one annoying issue (MSVC related, as usual) with doing it that way: MSVC considers all the constexpr variables unused if only used in the static_assert, and so issues a warning which breaks the build, and so separate variables would need a (void) (compiletime_bases, runtime_bases, needs_mi); to silence it, which is a bit ugly.

I thought it might also end up being potentially less work for the compiler this way: typically the number of base classes is 0, so short-circuiting avoids needing to compute everything but the first one.

[jagerman]

Given the MSVC warning bug, I think I'll merge as is.

[dean0x7d]

Sure, better than adding workarounds.

[jagerman]

Rebased to make sure this works with MSVC 2017 (MSVC 2015u3 was very fragile around the static_assert: changing the constexpr_sum(..) == 0 bit to a none_of<..> made it sometimes fail to compile with a "not a constexpr value" error, and other times ICE at link time, depending on how exactly I wrote the expression.)