Skip to content

Conversation

jfirebaugh
Copy link
Contributor

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
Copy link
Contributor

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:

  • ban implicit conversions, bump to 2.0, come to consensus that the design goals must shift
  • allow implicit conversions, warn against using them in the readme
  • add a MACRO to enable or disable them (support both modes)

@jfirebaugh
Copy link
Contributor Author

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 jfirebaugh force-pushed the no-implicit-conversions branch from 5bdca9f to 98b427b Compare May 10, 2016 18:24
@jfirebaugh
Copy link
Contributor Author

Continuing discussion on #100.

@jfirebaugh jfirebaugh closed this May 10, 2016
@jfirebaugh jfirebaugh deleted the no-implicit-conversions branch May 10, 2016 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants