Useful traits and Boost.Spirit Qi/Karma support headers for mapbox::variant.

[daminetreg]

Dear Mapbox Variant authors,

I would like to contribute support headers for Boost.Spirit.Qi and Karma to mapbox::variant, so that usage of Mapbox::variant with Qi and Karma get really easy.

Why ?

• Currently if you have a really large variant type (e.g. more as 50) with Boost.Spirit you need to re-preprocess the Boost.Mpl headers to enable your type to be compiled with Boost.Variant.
• Meaning you need a Patched Boost. With all the problems it brings when you are a lib publisher.
• Bigger mpl::vector<> for a bigger Boost.Variant makes the compile times of qi::rule and karma::rule decrease significantly.
• It also has strong impact on binary sizes, that with MapBox::Variant gets away. 😄

small test program with	Boost.Variant	MapBox.Variant
Boost.Spirit Qi	75 Kb	63 Kb
Boost.Spirit Karma	119 Kb	83 Kb

See benchmark code here

So I updated a big codebase which was using Boost.Variant and Boost.Spirit together with Mapbox::variant. And now it compiles faster, makes smaller binaries and we don't need a patched Boost MPL anymore ! 😀 Thank you very much for this variant type.

Features

The small supporting headers I wrote might be useful for users of this library, therefore I would like to contribute the following to Mapbox::Variant :

1. A metafunction to detect whether some type is a mapbox::variant
2. A metafunction to check if a type is part of a variant.
3. One supporting header for Boost.Spirit.Qi, defining the required traits
4. One supporting header for Boost.Spirit.Karma, defining the required traits.

I would naturally understand that the supporting header for Qi and Karma are not interesting you as part of the library, as they base on the Boost.Spirit traits which are documented only in spirit code but which are used extensively internally by the library and provided as supporting headers since ages (https://github.com/boostorg/spirit/blob/develop/include/boost/spirit/home/support/extended_variant.hpp).

Following if you are interested in the supporting headers for Boost.Spirit or not I will publish them as a separate library and remove 3. and 4. from this PR.

But I really would love to see the include/mapbox/traits/is_mapbox_variant.hpp and include/mapbox/traits/is_type_in_variant.hpp accepted, as they are in my opinion features that should be part of Mapbox Variant.

You can test out the supporting headers by running :

• make out/boost_spirit_karma && out/boost_spirit_karma
• make out/boost_spirit_qi && out/boost_spirit_karma

Cheers

[daniel-j-h]

Out of interest this does not work with Boost.Spirit X3, does it?

[daminetreg]

No this works only for Boost.Spirit Qi and Karma, X3 being still in development. And X3 can only parse, it cannot generate. It doesn't replace karma for the moment.

[artemp]

@daminetreg - thanks for the PR! Using mapbox::variant with boost::spirit is what (at least partially) motivated me in the first place. So far I was adapting mapbox::variant to play nice with Spirit.QI/Karma/X3 via struct adapted_variant_tag; and exposing using types = std::tuple<Types...>;

as in https://github.com/mapnik/mapnik/blob/master/include/mapnik/util/variant.hpp#L34-L43

Your non-intrusive approach has its merits, indeed. I'll comment more once I'll have time to dig this PR properly.

BTW, supporting boost::spirit::x3 would just require specialising an extra bunch of traits or am I missing something ? /cc @daniel-j-h

[daminetreg]

@daminetreg - thanks for the PR! Using mapbox::variant with boost::spirit is what (at least partially) motivated me in the first place. So far I was adapting mapbox::variant to play nice with Spirit.QI/Karma/X3 via struct adapted_variant_tag; and exposing using types = std::tuple<Types...>;
as in https://github.com/mapnik/mapnik/blob/master/include/mapnik/util/variant.hpp#L34-L43

The problem is that this approach works well for Qi. But is not enough for Karma, because Karma is less well designed than Qi to accept other variant type. Or did I miss something ?

Therefore the hack here : https://github.com/mapbox/variant/pull/119/files#diff-e54bc111e39a8bcc99b0fa543bd5ea0bR10

I couldn't see anyway to override this implementation from Karma : https://github.com/boostorg/spirit/blob/develop/include/boost/spirit/home/karma/detail/alternative_function.hpp#L133

BTW, supporting boost::spirit::x3 would just require specialising an extra bunch of traits or am I missing something ?

I can try it, there is a wiki page on mapbox about this anyway. So I'll dig into adding x3 support. 😄

[artemp]

@daminetreg - good point re:boost.karma, I recall now I ended up with rather verbose implementation to dispatch based on a stored type in variant : https://github.com/mapnik/mapnik/blob/master/include/mapnik/wkt/wkt_generator_grammar_impl.hpp#L59-L83

[daminetreg]

What is the difference between mapnik::util::variant and mapbox::util::variant ? Is it a fork ?

Do you think your intrusive approach has chances to be better accepted inside mapbox::variant ? If I understand it well it works great for X3 already ?

If so and if we can get it working seamlessly for karma, I'm fully happy with it. What we woud love @sauter-hq and personally is to simply don't bother and use mapbox::variant instead of Boost.Variant so long Boost Variant is not fully C++1z variadic.

Thank you for reviewing my changes ! 😄

[artemp]

What is the difference between mapnik::util::variant and mapbox::util::variant ? Is it a fork ?

Nope, mapnik::util::variant inherits from mapbox::util::variant and adds struct adapted_variant_tag and types in order to work with boost::spirit (https://github.com/mapnik/mapnik/tree/master/deps/mapbox)

Do you think your intrusive approach has chances to be better accepted inside mapbox::variant ? If I understand it well it works great for X3 already ?

Yes, I would accept it. This is on my TODO list. As I mentioned I would also like to evaluate your non-intrusive approach.

If so and if we can get it working seamlessly for karma, I'm fully happy with it.

This is what needs some evaluation ^ I'm recalling karma support is not fully functional with current intrusive approach