search

mapbox / variant

C++11/C++14 Variant
BSD 3-Clause "New" or "Revised" License
371 stars 100 forks source link

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

Open daminetreg opened 8 years ago

daminetreg commented 8 years ago

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 ?

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 ! :grinning: 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 :

Cheers

daniel-j-h commented 8 years ago

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

daminetreg commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

@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; 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. :smile:

artemp commented 8 years ago

@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 commented 8 years ago

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 ! :smile:

artemp commented 8 years ago

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