Page MenuHomePhabricator
Differential D97394 Diff 332916 libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp

Changeset View

Standalone View

View Options

libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp

Show First 20 Lines • Show All 342 Lines • ▼ Show 20 Linesvoid test_caller_accepts_nonconst() {
struct A {}; struct A {};
struct Visitor { struct Visitor {
void operator()(A&) {} void operator()(A&) {}
}; };
std::variant<A> v; std::variant<A> v;
std::visit(Visitor{}, v); std::visit(Visitor{}, v);
} }
struct MyVariant : std::variant<short, long, float> {};
namespace std {
template <size_t Index>
void get(const MyVariant&) {
assert(false);
}
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

From the outside, it seems vastly more important to verify that visit does not take std::get<Idx>(v) specializations from the base class. Could you add a test for that?
Of course it probably isn't necessary to test all the possible entry-points, right? they should all work pretty much the same?

Quuxplusone: From the outside, it seems vastly more important to verify that `visit` does not take `std…
curdeiusAuthorUnsubmitted
Done
ReplyInline Actions

Not sure if I correctly understood what test you suggested. Could you have a look please?

curdeius: Not sure if I correctly understood what test you suggested. Could you have a look please?
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

Yes, that's what I meant. :) Except let's make one of those MyVariant arguments a const reference too, for extra overload-resolution matchy-matchiness.

const MyVariant v1 = 42;
std::visit([](auto x) { assert(x == 42); }, v1);
std::visit([](auto x) { assert(x == -1.25f); }, MyVariant(-1.25f));

Drive-by changed the float to a value that can be represented exactly in base-2, because I'm paranoid about floats. :)

Quuxplusone: Yes, that's what I meant. :) Except let's make one of those MyVariant arguments a const…
} // namespace std
void test_derived_from_variant() {
std::visit([](auto x) { assert(x == 42); }, MyVariant{42});
std::visit([](auto x) { assert(x == -1.3f); }, MyVariant{-1.3f});
// Check that visit does not take index nor valueless_by_exception members from the base class.
struct EvilVariantBase {
int index;
char valueless_by_exception;
ldionneUnsubmitted
Done
ReplyInline Actions

Maybe add a short comment explaining what we expect and why it's useful to test this? I like to put myself in the shoes of my future self when I don't have the paper in front of me - I wouldn't understand why this test exists. If nothing else, perhaps mentioning the paper number is sufficient.

ldionne: Maybe add a short comment explaining what we expect and why it's useful to test this? I like to…
curdeiusAuthorUnsubmitted
Done
ReplyInline Actions

Will do.

curdeius: Will do.
};
struct EvilVariant1 : std::variant<int, long, double>,
std::tuple<int>,
EvilVariantBase {
curdeiusAuthorUnsubmitted
Done
ReplyInline Actions

s/pick/picks/.
Will fix typos locally for the moment. Here and in relops.

curdeius: s/pick/picks/. Will fix typos locally for the moment. Here and in relops.
using std::variant<int, long, double>::variant;
};
std::visit([](auto x) { assert(x == 12); }, EvilVariant1{12});
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

Would it be appropriate to also test the behavior of struct X : std::variant<int>, std::variant<long> {} and struct Y : private std::variant<int> {}? I don't think we expect either of those to support std::visit, but I wonder if they're supposed to SFINAE away cleanly or give some awful template error message or what.

Quuxplusone: Would it be appropriate to also test the behavior of `struct X : std::variant<int>, std…
curdeiusAuthorUnsubmitted
Done
ReplyInline Actions

I don't see how it may be SFINAE-friendly (but I haven't checked).

curdeius: I don't see how it may be SFINAE-friendly (but I haven't checked).
curdeiusAuthorUnsubmitted
Done
ReplyInline Actions

Well, I've checked those and they fail with more or less cryptic messages, but still I find them pretty nice.
E.g.

struct BadVariant : std::variant<int>, std::variant<double> {
  using std::variant<int>::variant;
};
std::visit([](auto x) { assert(x == 12); }, BadVariant{12});

fails with:

/home/marek/llvm-project/libcxx/include/variant:1669:36: error: no matching function for call to '__as_variant'
  const bool __valueless = (... || _VSTD::__as_variant(__vs).valueless_by_exception());
                                   ^~~~~~~~~~~~~~~~~~~
/home/marek/llvm-project/libcxx/include/__config:779:15: note: expanded from macro '_VSTD'
#define _VSTD std::_LIBCPP_ABI_NAMESPACE
              ^
/home/marek/llvm-project/libcxx/include/variant:1680:10: note: in instantiation of function template specialization 'std::__throw_if_valueless<BadVariant>' requested here
  _VSTD::__throw_if_valueless(_VSTD::forward<_Vs>(__vs)...);
         ^
/home/marek/llvm-project/libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:397:8: note: in instantiation of function template specialization 'std::visit<(lambda at /home/marek/llvm-project/libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:397:14), BadVariant>' requested here
  std::visit([](auto x) { assert(x == 12); }, BadVariant{12});
       ^
/home/marek/llvm-project/libcxx/include/variant:326:1: note: candidate template ignored: failed template argument deduction
__as_variant(variant<_Types...>& __vs) noexcept {
^
/home/marek/llvm-project/libcxx/include/variant:332:1: note: candidate template ignored: failed template argument deduction
__as_variant(const variant<_Types...>& __vs) noexcept {
^
...

I think there's nothing we can do about it.

curdeius: Well, I've checked those and they fail with more or less cryptic messages, but still I find…
tcanensUnsubmitted
Done
ReplyInline Actions

The wording is pretty clear that this case needs to SFINAE.

tcanens: The wording is pretty clear that this case needs to SFINAE.
curdeiusAuthorUnsubmitted
Done
ReplyInline Actions

Right. I might need to test it in a different manner though.

curdeius: Right. I might need to test it in a different manner though.
std::visit([](auto x) { assert(x == 12.3); }, EvilVariant1{12.3});
// Check that visit unambiguously picks the variant, even if the other base has __impl member.
ldionneUnsubmitted
Done
ReplyInline Actions

Is there a reason why you're only calling those in an unevaluated context? Can't we actually call and "run" them (even though I understand it's trivial)?

ldionne: Is there a reason why you're only calling those in an unevaluated context? Can't we actually…
tcanensUnsubmitted
Done
ReplyInline Actions

Constructing the type_info base class might be a bit tricky...

tcanens: Constructing the `type_info` base class might be a bit tricky...
curdeiusAuthorUnsubmitted
Done
ReplyInline Actions

Indeed, that was the reason why I just used unevaluated context.
I'll try to do something else instead, because the reason for using type_info that both variant and type_info has __impl member that is ambiguous when calling visit (prior to this patch).

curdeius: Indeed, that was the reason why I just used unevaluated context. I'll try to do something else…
ldionneUnsubmitted
Done
ReplyInline Actions

Oh, I hadn't thought of that. Yes, that's obvious now :-).

Feel free to disregard this comment and leave it unevaluated, or to do whatever you were thinking about with a hand-made class.

ldionne: Oh, I hadn't thought of that. Yes, that's obvious now :-). Feel free to disregard this comment…
struct ImplVariantBase {
struct Callable {
bool operator()();
};
Callable __impl;
};
struct EvilVariant2 : std::variant<int, long, double>, ImplVariantBase {
using std::variant<int, long, double>::variant;
};
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

Nit: mark this operator() as const, for good style.

Quuxplusone: Nit: mark this `operator()` as `const`, for good style.
std::visit([](auto x) { assert(x == 12); }, EvilVariant2{12});
std::visit([](auto x) { assert(x == 12.3); }, EvilVariant2{12.3});
}
struct any_visitor {
template <typename T>
void operator()(const T&) const {}
};
template <typename T, typename = decltype(std::visit(
std::declval<any_visitor&>(), std::declval<T>()))>
constexpr bool has_visit(int) {
return true;
}
template <typename T>
constexpr bool has_visit(...) {
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

Nit: > > should be >> since this test needn't run in C++03 mode.

Non-nit: Please write 0, not int().

Quuxplusone: Nit: `> >` should be `>>` since this test needn't run in C++03 mode. Non-nit: Please write `0`…
return false;
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

Please also static_assert(!has_visit<int>).
Please also test struct BadVariant2 : private std::variant<long, float> {} (!has_visit).
Please also test struct GoodVariant : public std::variant<long, float> {} (has_visit).
Optionally also test struct GoodVariant2 : public GoodVariant {} (has_visit).

Quuxplusone: Please also `static_assert(!has_visit<int>)`. Please also test `struct BadVariant2 : private…
}
void test_sfinae() {
struct BadVariant : std::variant<short>, std::variant<long, float> {};
struct BadVariant2 : private std::variant<long, float> {};
struct GoodVariant : std::variant<long, float> {};
struct GoodVariant2 : GoodVariant {};
static_assert(!has_visit<int>(0));
static_assert(!has_visit<BadVariant>(0));
static_assert(!has_visit<BadVariant2>(0));
static_assert(has_visit<std::variant<int> >(0));
QuuxplusoneUnsubmitted
Done
ReplyInline Actions

nit: >> is OK here

Quuxplusone: nit: `>>` is OK here
static_assert(has_visit<GoodVariant>(0));
static_assert(has_visit<GoodVariant2>(0));
}
int main(int, char**) { int main(int, char**) {
test_call_operator_forwarding(); test_call_operator_forwarding();
test_argument_forwarding(); test_argument_forwarding();
test_return_type(); test_return_type();
test_constexpr(); test_constexpr();
test_exceptions(); test_exceptions();
test_caller_accepts_nonconst(); test_caller_accepts_nonconst();
test_derived_from_variant();
test_sfinae();
return 0; return 0;
} }