Access via type

Our implementation of variant allows access via get<type>() even if the type appears several times in the variant. This can be potentially confusing and error prone, especially when type aliasing is involved. Say, you got a variant variant<int32_t, int>. Do you know what get<int>() will do? Does the architecture matter?

What makes things even more confusing is the addition of special versions of get that unwrap std::reference_wrapper and mapbox::util::recursive_wrapper on the fly. And they get accessed using the underlying type. So having a variant<recursive_wrapper<int>> you can access that int using the somewhat magical get<int>(). What happens when you access a variant<int, recursive_wrapper<int>> using get<int>()?

I do see the convenience these choices bring, but on the other hand, these could lead to really hard to find bugs. The upcoming standards "solves" the first issue by not allowing get-access through the types if the types are not unique. You have to use index-based access then, which we don't have. It doesn't have the second problem because it doesn't have those magic wrappers.

There's another issue: visitors. Once two alternatives become the same type, a visitor overloading operator() on both won't compile. That means if you have alternatives that may be the same type on some architecture, you either need to wrap them in a tag struct, or overload visitation on type index. Neither seems ideal to me, but the former can be done even if variant forbids identical types.

re: get<T> - Lets not over engineer here, at least not now :). As stated in https://github.com/mapbox/variant/blob/master/README.md the goal is :

"..is to maintain external API compatibility with boost::variant such that Mapbox variant can be a "drop in".

Adding more compile checks will increase compile times for not much of a value in my view. Happy to revisit this later when std variant becomes a reality.

Both our variant and boost::variant have well defined behaviour:

#include <iostream>

#include <boost/variant.hpp>
#include <variant.hpp>

int main()
{
    std::cerr << "testing" << std::endl;

    { // boost                                                                                                                                                  
        boost::variant<int,double,int> v;
        v=123;
        std::cerr << v.which() << std::endl;
        std::cerr << boost::get<int>(v) << std::endl;
    }
    { // mapbox                                                                                                                                                 
        mapbox::util::variant<int,double,int> v;
        v=123;
        std::cerr << v.which() << std::endl;
        std::cerr << mapbox::util::get<int>(v) << std::endl;
    }
    return 0;
}

./test 
testing
0
123
0
123

Perhaps well-defined, but very dangerous, imo.

using foo_t = libfoo::unspecified_object_handle_type;
using bar_t = libbar::unspecified_object_handle_type;
using var = variant<int, std::string, foo_t, bar_t>;

struct is_scalar_visitor
{
    bool operator() (int) const { return true; }
    bool operator() (std::string const&) const { return true; }
    template <typename T> bool operator() (T const&) const { return false; }
};

struct get_length_visitor
{
    int operator() (std::string const& s) const { return int(s.length()); }
    int operator() (foo_t const& o) const { return libfoo::get_obj_length(o); }
    // other types don't have a length
    template <typename T> int operator() (T const& ) const { return -1; }
};

As long as libfoo and libbar use something unique, like a pointer to an opaque struct for their handles, everything works fine. The day either of them switches to using an int or std::string, or when they start using the same type for handle, like void*, all hell will break loose.

Sure you can write safer visitors with no template fallbacks. Then they won't compile. All in all, having same-type alternatives in variant is pretty useless in my opinion, and dangerous. It should be disallowed at class instantiation level -- unless we want to provide indexed get<I> like std.

@lightmare

@lightmare - I agree, "same-type alternatives" are pretty useless. My only concern is that compile check can be relatively expensive (thinking about mapnik::expression_node -https://github.com/mapnik/mapnik/blob/master/include/mapnik/expression_node_types.hpp#L169-L196).
How would you propose checking type uniqueness at compile time?

Found this one : http://stackoverflow.com/questions/18986560/check-variadic-templates-parameters-for-uniqueness

@artemp

@artemp Oh, haven't thought about complexity. You're right, it'd be something like direct_type over 2 axes, O(n**2).

Maybe some optimization ideas are in http://www.boost.org/doc/libs/1_60_0/libs/mpl/doc/refmanual/set.html

The best I can come up with is:

// True if Predicate<F, T> matches for none of the types T from Ts
template <template<typename, typename> class Predicate, typename F, typename... Ts>
struct static_none_of_binary : std::is_same<std::tuple<std::false_type, typename Predicate<F, Ts>::type...>,
                                            std::tuple<typename Predicate<F, Ts>::type..., std::false_type>>
{};

template <typename... Ts>
struct is_unique;

template <typename T1, typename T2, typename... Ts>
struct is_unique<T1, T2, Ts...> {
    static constexpr const bool value = static_none_of_binary<std::is_same, T1, Ts...>::value &&
                                        is_unique<T2, Ts...>::value;
};

template <typename T1>
struct is_unique<T1> {
    static constexpr const bool value = true;
};

At least the inner check isn't recursive this way. We'd probably need to benchmark compiling to be sure, though.

@joto

@joto - great, will need to bench it! How about making this check an optional (default: yes) ? This way it'll protect new users and also will allow power users to turn it off for large type sequences ?

/cc @lightmare

@joto

planning to bench finally
/cc @joto @springmeyer @lightmare

Navigation Menu

Repository navigation

Access via type #84

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Access via type #84

Description

Activity

lightmare commented on Jan 29, 2016

artemp commented on Feb 1, 2016

lightmare commented on Feb 1, 2016

artemp commented on Feb 1, 2016

artemp commented on Feb 1, 2016

lightmare commented on Feb 1, 2016

artemp commented on Feb 1, 2016

joto commented on Feb 1, 2016

artemp commented on Feb 1, 2016

artemp commented on Jan 6, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions