Allow arbitrary class_ template option ordering

[jagerman]

The current pybind11::class_<Type, Holder, Trampoline> fixed template ordering results in a requirement to repeat the Holder with its default value (std::unique_ptr<Type>) argument, which is a little bit annoying: it needs to be specified not because we want to override the default, but rather just because we need to specify the third argument.

This commit removes this limitation by making the class_ template take the type name plus a parameter pack of options. It then extracts the first valid holder type and the first subclass type for holder_type and trampoline type_alias, respectively. (If unfound, both fall back to their current defaults, std::unique_ptr<type> and type, respectively). If any unmatched template arguments are provided, a
static assertion fails.

What this means is that you can specify or omit the arguments in any order:

    py::class_<A, PyA> c1(m, "A");
    py::class_<B, PyB, std::shared_ptr<B>> c2(m, "B");
    py::class_<C, std::shared_ptr<C>, PyB> c3(m, "C");

It also means that other class attributes can be passed this way—for example, the base type (in the next commit), or the always_copy class attribute being discussed in #376.

[jagerman]

(If unfound, both fall back to their current defaults, std::unique_ptr<type> and type, respectively)

Actually, that isn't quite right--type_alias is set to void if no trampoline was specified, but since it wasn't available outside the class at all before, this won't change anything.

[wjakob]

Removing the order requirement in the class_ arguments is nifty, and it nicely corresponds to similar behavior in parameter order independence in class_() and def().

That said, I'm on the fence for this one for a couple of reasons:

With this patch, there would now be two 100% equivalent ways of specifying the base class (either as a variadic template argument, or via py::base<>). As was discussed on the tracker a few times, a longer term plan of mine is to supplement py::base<> with py::bases<> for multiple inheritance (which is only instantiated when truly needed, hence ensuring compact code generation for the simpler single inheritance case). It was not clear to me when reading your patch that these things would be straightforward to do.

The amount of metatemplate programming that is necessary to pull this off makes for a gratuitously implementation. I expect that this also has an adverse influence on compilation time for big projects, where class_ and members of class_ are instantiated many times). Even if that is not the case, I am hesitant to introduce this much complexity.

[dean0x7d]

The template logic could be significantly simplified with something like this:

template<template<class> class Predicate, class Default, class... Ts>
struct extract_if;

template<template<class> class Predicate, class Default>
struct extract_if<Predicate, Default> { using type = Default; };

template<template<class> class Predicate, class Default, class T, class... Ts>
struct extract_if<Predicate, Default, T, Ts...> : std::conditional<
    Predicate<T>::value, T, typename extract_if<Predicate, Default, Ts...>::type
> {};

// Extract the first type that satisfies the Predicate or return Default
template<template<class> class Predicate, class Default, class... Ts>
using extract_if_t = typename extract_if<Predicate, Default, Ts...>::type;

Example usage:

using holder_type = extract_if_t<is_holder, default_holder, options...>;
using type_alias = extract_if_t<is_subtype, void, options...>;

This also avoids dragging in std::tuple's template machinery which isn't very lightweight.

[jagerman]

@dean0x7d the point of the std::tuple bits was to be able to provide a list of unmatched parameters, to pass through to the next function, and to verify that, after getting out everything, there are no extra parameters. Perhaps the first isn't necessary, though (it seems highly unlikely that a holder class will also be a subclass), and the second ought to be doable by counting the number of matches. I'll investigate.

[dean0x7d]

You can pair the above extract_if_t with an all_of_t (from PR #372) to make sure all template parameters are valid:

static_assert(all_of_t<is_valid_class_option, options...>::value,
              "Unknown/invalid class_ template parameters provided");

[jagerman]

I was thinking of just doing a count instead:

    static_assert(0 == sizeof...(options) - (int) has_alias - (int) extracted_holder::found - (int) extracted_base::found,
            "Unknown/invalid class_ template parameters provided");

[wjakob]

found is probably not needed, can use std::is_same<...::type, void> below.

[wjakob]

This looks simpler now, though I'd still like to run some gcc/clang benchmarks to be sure that this does not unduly increase compilation time. The question about multiple bases remains though.

[dean0x7d]

I think that going with all_of_t would further improve readability and cut down on on intermediate metavariables -- there would be no need for the ::found bits and the defaults could be handled by adding a Default param in extract_first. At that point, that most basic case of class_<T> without any options would have very little metaprogramming overhead since an empty parameter pack options... would take the simple default path.

[jagerman]

I think that going with all_of_t would further improve readability and cut down on on intermediate metavariables

Unless I'm missing something, wouldn't it require another is_valid_class_option predicate to be defined? It also would be incapable of detecting duplicate options; currently you get a compilation error if you accidentally specify a holder twice, or give two trampoline classes.

[jagerman]

It also would be incapable of detecting duplicate options; currently you get a compilation error if you accidentally specify a holder twice, or give two trampoline classes.

Maybe that's not a big deal. If you specify multiple trampolines, or multiple holders, you get undefined behaviour (in practice, everything past the first would be ignored).

[jagerman]

The question about multiple bases remains though.

I'm thinking about it, will reply soon.

[jagerman]

(The obvious thing here is to be able to specify py::class_<Type, MIBase1, MIBase2>, I'm just trying to work out a nice way to extract that).

[dean0x7d]

It also would be incapable of detecting duplicate options

Yeah, I didn't consider duplicates -- it wouldn't catch those, but I'm not sure how important that is.

One other thing to consider: wrapping class_ options may improve readability:

py::class_<C, py::alias<PyC>, py::bases<A, B>> c(m, "C");
// vs
py::class_<C, PyC, A, B> c(m, "C");

It would also make it simpler to add any future option to class_ since the wrappers are unique.

(Although, I'm not quite sure why there are 2 ways of specifying bases by C++ type name: template param and constructor param.)

[jagerman]

See the last commit: it allows specification of multiple base classes, currently producing an assertion failure if you specify more than one base. Eventual MI support would then just be a matter of uncommenting the class_bases<Base1, Base2, Bases...> specialization and giving it an appropriate add_to body that records the relationship.

[jagerman]

(Although, I'm not quite sure why there are 2 ways of specifying bases by C++ type name: template param and constructor param.)

Well, I saw this as a replacement for the py::base<B>() constructor parameter, because it seems a bit more consistent to have all the C++ types in the template parameters, rather than having some (holder/trampoline) there, and some curried through an attribute class.

That said, the py::base<B>() obviously needs to stick around for backwards compatibility. (Multiple inheritance, on the other hand, could just use this without needing a py::bases<...> forwarding attribute).

[jagerman]

Yeah, I didn't consider duplicates -- it wouldn't catch those, but I'm not sure how important that is.

You're probably right. Switching to all_of_t would be fine then (assuming the extra predicate doesn't affect compilation size).

[dean0x7d]

(Multiple inheritance, on the other hand, could just use this without needing a py::bases<...> forwarding attribute).

On the other other hand, if py::bases<...> were used there would no need for the extract_all metafunction.

assuming the extra predicate doesn't affect compilation size

Predicates in the form of template aliases are extremely cheap (dare I say free). It's preferable to use an alias whenever possible:

template<class T> using is_my_class = is_same<MyClass, T>; // alias
// vs.
template<class T> struct is_my_class : is_same<MyClass, T> {}; // new type

The alias is much lighter weight than the struct. The latter will instantiate a new type and the compiler needs to be aware of it (uses compiler memory and time). The alias is essentially just a replacement: it does not introduce a new type, thus it cannot be specialized and it does not appear as a unique type for overloads or class specializations.

[jagerman]

Predicates in the form of template aliases are extremely cheap (dare I say free)

Oops, I mean compilation speed, not size.

[dean0x7d]

Oops, I mean compilation speed, not size.

I assumed so. Both are compile to nothing. The post above applies to compile performance.

[aldanor]

Yea, this looks significantly simpler now, and I believe the result is worth the hackery, more extendable in case more template parameter stuff will be added in the future.

[jagerman]

The alias is much lighter weight than the struct.

Yes, I use it when possible, but as far as I can see, it can't be used as a partial specialization, along the lines of:

template <typename T> class A {};
template <> using A<bool> = std::true_type;

[jagerman]

I suppose I could do something like:

template <typename T> class A_impl {};
template <typename T> using A = typename std::conditional<std::is_same<T, bool>::value, std::true_type, A_impl<T>>::type;

would this still be worthwhile over:

template <typename T> struct A {};
template <> struct A<bool> : std::true_type {};

?

[dean0x7d]

If you need specialization, go with the struct. Readability is still important, and besides, the std::conditional would end up doing a similar instantiation behind the scenes anyway.

[jagerman]

Tagging this WIP as I'm still playing around with the implementation a little and running some compiler benchmarks. I think it'll also depend on PR #372 (to use all_of_t for the assert, as @dean0x7d suggested a few comments ago).

[jagerman]

I've based this now on PR #372 for its all_of_t template. There's a fair bit of squashing/rebasing to do, but I'll wait until after #372 is merged.

I also got rid of the std::tuple approach for the base classes--it seems to be more compiler friendly in its current form, where it simple iterates through all the options until it find a base class.

[jagerman]

@dean0x7d Any idea if there's a nicer way to work around (9e240eb) for MSVC than what I did there?

[jagerman]

@wjakob I've uploaded some benchmarking results using g++ and clang++ on linux compiled under -O2, using the benchmark code in tools/bench that I added in this PR, comparing this branch against PR #372 (which is currently merged into this branch).

These are pathologically bad scenarios (100 to 1000 registered, empty classes declared in a single compilation unit, without anything else declared--just the classes), just to get an idea for the absolute maximum worst case. The quick takeaway is that the compilation time increase is around 0-3%, depending on the case. The worst case for compiler memory use is about 300KB per registered class under g++, and considerably less than that under clang++. That amount seems pretty unlikely to be a noticeable increase when you have many fewer classes and many more (i.e. more than 0) methods, functions, etc.. It's also the "compatibility" case, i.e. where you're uselessly specifying the default unique_ptr; if you omit it, compilation becomes faster and requires less memory.

[wjakob]

Thank you for benchmarking -- those numbers seem quite reasonable. Since it looks like you are still simplifying the template implementation and waiting for #372, I'll merge that when it's ready and then do another pass over your submission.

[dean0x7d]

@jagerman I updated the implementation of all_of_t in #372 and included a workaround for MSVC so you can use it as expected with the alias predicates. On a frustrating note, this MSVC bug is actually freshly broken in VS2015 Update 3 and doesn't happen in Update 2 and earlier.

[wjakob]

If it's easy to package up in a self-contained example, it may be worth submitting it on Microsoft connect. They are usually pretty good at fixing things (thought it will likely take until the next point release in that case)

[dean0x7d]

Yeah, I think a reproducible example would actually be quite compact. I'll put something together and submit it.

[wjakob]

FYI: #372 is merged now.

[jagerman]

Yeah, I'm working on the rebase right now.

[jagerman]

Rebased and squashed.

I included the benchmarking scripts I wrote to benchmark this (in tools/bench) because I think they might well be useful for testing similar features in the future.

[jagerman]

@dean0x7d Indeed the workaround is no longer needed (and removed from the squashed commits).

[jagerman]

As for multiple inheritance: it should be dead-easy to add to the interface now to allow specification via class_<Class, Base1, Base2, Base3>: all that is needed is to change class_selector::set_bases() to record any inheriting predicate-satisfying classes as bases instead of just setting the first one as the single base.

[dean0x7d]

You can inherit directly from is_base_of and skip conditional:

struct is_holder_type : std::is_base_of<...> {};

[jagerman]

Indeed that was pointlessly complex.

[jagerman]

Squashed those suggestions (but keeping has_alias).

[wjakob]

I think it would be better to separate out the benchmarking script, which would benefit from some more discussion.

[wjakob]

Other than that, this looks great now -- the readability is much better, and the new infrastructure will be useful for other parts of the library as well.

[jagerman]

Okay, the benchmarking commit is removed from the PR.

[wjakob]

Great, thank you!