mapbox / variant

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

Disallow implicit conversions in constructor and assignment #103

Closed jfirebaugh closed 8 years ago

jfirebaugh commented 8 years ago

Fixes #100.

This should be considered a breaking change, to be released as 2.0. I can hit some of the other 2.0 changes like removing deprecated parts and tagged issues.

cc @artemp @springmeyer @joto

springmeyer commented 8 years ago

If I recall correctly, this is the way variant was implemented originally (@artemp preferred requiring explicit types). Implicit conversions I think were added at the request of OSRM devs at the time to enable variant being a drop-in replacement for boost::variant (which allows implicit conversions) (which is a design goal https://github.com/mapbox/variant#goals).

So the options I see are:

jfirebaugh commented 8 years ago

boost::variant protects against unexpected ambiguous conversions where mapbox::util::variant does not:

This compiles and prints 0 (1 was likely expected):

    mapbox::util::variant<bool, uint64_t> mbv(1234);
    std::cout << mbv.which();

This does not compile:

    boost::variant<bool, uint64_t> bv(1234);
    std::cout << bv.which();
/Users/john/Development/variant/mason_packages/headers/boost/1.60.0/include/boost/variant/variant.hpp:1558:28: error: call to member function 'initialize' is ambiguous
              initializer::initialize(
              ~~~~~~~~~~~~~^~~~~~~~~~
/Users/john/Development/variant/mason_packages/headers/boost/1.60.0/include/boost/variant/variant.hpp:1735:9: note: in instantiation of function template specialization
      'boost::variant<bool, unsigned long long>::convert_construct<int>' requested here
        convert_construct( detail::variant::move(operand), 1L);
        ^
test/bench_variant.cpp:186:36: note: in instantiation of function template specialization 'boost::variant<bool, unsigned long long>::variant<int>' requested here
    boost::variant<bool, uint64_t> bv(1234);
                                   ^
jfirebaugh commented 8 years ago

Continuing discussion on https://github.com/mapbox/variant/issues/100.