Open joto opened 8 years ago
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 - 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?
@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 - 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
planning to bench finally /cc @joto @springmeyer @lightmare
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 variantvariant<int32_t, int>
. Do you know whatget<int>()
will do? Does the architecture matter?What makes things even more confusing is the addition of special versions of
get
that unwrapstd::reference_wrapper
andmapbox::util::recursive_wrapper
on the fly. And they get accessed using the underlying type. So having avariant<recursive_wrapper<int>>
you can access thatint
using the somewhat magicalget<int>()
. What happens when you access avariant<int, recursive_wrapper<int>>
usingget<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.