Page MenuHomePhabricator

[libcxx] Add <variant> tests (but not implementation)
ClosedPublic

Authored by EricWF on Nov 20 2016, 4:54 PM.

Details

Summary

Although libc++ doesn't yet implement variant we do have tests!

@STL_MSFT @CaseyCarter IIRC you already have a <variant> implementation. Would you mind checking these tests against your implementation?

Diff Detail

Event Timeline

EricWF updated this revision to Diff 78677.Nov 20 2016, 4:54 PM
EricWF retitled this revision from to [libcxx] Add <variant> tests (but not implementation).
EricWF updated this object.
EricWF added reviewers: STL_MSFT, CaseyCarter.
CaseyCarter edited edge metadata.Nov 20 2016, 6:41 PM

I'll take a look at these tomorrow; I don't have easy access to my implementation with the changes from P0510 and P0504 from home (it's waiting on a compiler bugfix to checkin).

test/std/utilities/variant/variant.get/get_type.pass.cpp
19

Space at end-of-line here.

EricWF updated this revision to Diff 78686.Nov 20 2016, 7:45 PM
EricWF edited edge metadata.
  • Fix a couple extra usages of variant<void> or variant<T&>.
  • Fix variant::hash SFINAE tests.
EricWF updated this revision to Diff 78710.Nov 21 2016, 5:55 AM
  • Attempt to fix TestTypes::NoCtors actually having constructors.
CaseyCarter added inline comments.Nov 21 2016, 1:01 PM
test/std/utilities/variant/variant.relops/relops.pass.cpp
61

MSVC with /W4 is, uh, not found of these functions. Adding "return false;" after the assert shuts it up.

190

Bogus test: should be assert(test_less(v1, v2, false, true));. Maybe someone forgot to implement part of P0032?

196

Bogus test again: should be assert(test_less(v1, v2, true, false));.

test/std/utilities/variant/variant.variant/variant.assign/copy.pass.cpp
150

All four of these are non-portable. Implementations are permitted to strengthen noexcept, and these are assignments that can feasibly be implemented to not throw.

test/std/utilities/variant/variant.variant/variant.mod/emplace_index_args.pass.cpp
93

y and z are unused here,

test/std/utilities/variant/variant.variant/variant.mod/emplace_type_args.pass.cpp
94

y and z are unused here.

test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp
322

Non-portable. Replace with LIBCPP_ASSERT(T::move_called == 1); assert(T::move_called <= 2);

329

Non-portable. Replace with LIBCPP_ASSERT(T::move_called == 2); assert(T::move_called <= 2);

350

Extra space before == here and on 370.

351

Non-portable. Replace with LIBCPP_ASSERT(T2::move_called == 1); assert(T2::move_called <= 1);

353

non-portable. s/b

if (T2::move_called != 0)
    assert(v2.valueless_by_exception());
else
    assert(std::get<1>(v2) == 100);
368

This line is a dup of 369. T1::move_called was probably intended, in which case it is non-portable and should be LIBCPP_ASSERT(T1::move_called == 1); assert(T1::move_called <= 1);

372

Non-portable. S/b:

if (T1::move_called != 0)
    assert(v1.valueless_by_exception());
else
    assert(std::get<0>(v1).value == 42);
376

This test and the next are non-portable and I can't be bothered to fix them.

404

I shouldn't have to point out that a test with the comment "testing libc++ extension" is non-portable ;)

406

guarantee

545

This should be LIBCPP_STATIC_ASSERT (SFINAE isn't mandated for member swap after LWG2749).

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

MSVC complains that all four of these operator() overloads name but do not use args; removing the name shuts up the warning.

STL_MSFT edited edge metadata.Nov 21 2016, 7:05 PM

Found some minor issues.

test/std/utilities/variant/variant.get/get_if_index.pass.cpp
93

Technically, addressof() lives in <memory>.

test/std/utilities/variant/variant.get/get_if_type.pass.cpp
53

<memory> for addressof() (although I'll note that the other tests are saying &v).

test/std/utilities/variant/variant.get/get_index.pass.cpp
177

Technically, <utility> for move().

221

<type_traits> for integral_constant.

test/std/utilities/variant/variant.get/get_type.pass.cpp
100

<utility> move()

test/std/utilities/variant/variant.hash/hash.pass.cpp
16

"<class ..._Types>" has really terrible spacing. (Or as I like to say, "Space. The final frontier.")

test/std/utilities/variant/variant.helpers/variant_alternative.pass.cpp
34

You aren't directly including <type_traits> (not sure if "variant_test_helpers.hpp" is).

61

The sense of this #if test seems to be flipped.

test/std/utilities/variant/variant.helpers/variant_size.pass.cpp
21

No indentation! :-<

36

Need <type_traits>.

42

Hmm, is this cromulent now that empty variants are forbidden?

test/std/utilities/variant/variant.monostate/monostate.pass.cpp
21

Don't actually need <cassert> since you have no plain asserts and this is C++17.

test/std/utilities/variant/variant.relops/relops.pass.cpp
60

No space after >, but space after < above. Madness!

67

<utility> move()

test/std/utilities/variant/variant.variant/variant.assign/copy.pass.cpp
29

NoCopy doesn't even have a default ctor, how are you supposed to make one? Appears to apply to the classes below.

test/std/utilities/variant/variant.variant/variant.assign/move.pass.cpp
115

"beth". Also, "move and move constructors"?

213

<utility> move()

test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp
66

You have like twenty different macros for enabling references.

test/std/utilities/variant/variant.variant/variant.ctor/in_place_index_init_list_Args.pass.cpp
1 ↗(On Diff #78710)

Should this test filename contain capital A?

test/std/utilities/variant/variant.variant/variant.ctor/in_place_type_Args.pass.cpp
23 ↗(On Diff #78710)

This file doesn't actually use <string>.

test/std/utilities/variant/variant.variant/variant.ctor/move.pass.cpp
61

Inconsistent indentation.

test/std/utilities/variant/variant.variant/variant.mod/emplace_index_args.pass.cpp
89

Why wrap here?

test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp
115

Postincrement? You monster!

287

happend

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

Technically, <memory> for addressof().

106

<utility> move()

203

Space. The final frontier. (Occurs below)

test/support/variant_test_helpers.hpp
54

Indentation

64

<type_traits>

Thanks guys! I'll address all these tomorrow!

CaseyCarter added inline comments.Nov 22 2016, 10:30 AM
test/std/utilities/variant/variant.hash/hash.pass.cpp
35

This is non-portable. The fact that both your implementation and mine pass this test suggests they both have poor QoI: There's strong opinion in the community that a variant's index should contribute to its hash value, so that e.g. variant<int, int>{in_place_index<0>, 42} and variant<int, int>{in_place_index<1>, 42} have differing hash values.

test/std/utilities/variant/variant.helpers/variant_size.pass.cpp
42

It's not forbidden to *name* variant<>; it is forbidden to *instantiate* it. The pertinent specialization of variant_size:

template <class... Types>
struct variant_size<variant<Types...>>
  : integral_constant<size_t, sizeof...(Types)> { };

is implicitly specified to not instantiate variant<Types...>.

EricWF added inline comments.Nov 22 2016, 10:47 AM
test/std/utilities/variant/variant.helpers/variant_size.pass.cpp
42

Note that there is no similar requirement that variant<void> or variant<T&> can even be named.

EricWF marked 45 inline comments as done.Nov 22 2016, 4:37 PM

Address almost all inline comments.

test/std/utilities/variant/variant.get/get_if_index.pass.cpp
93

I'm just changing it to &v.

test/std/utilities/variant/variant.helpers/variant_alternative.pass.cpp
34

Fixing this. Does your <variant> somehow not drag in <type_traits>?

test/std/utilities/variant/variant.variant/variant.assign/copy.pass.cpp
29

Don't care about making them. They are only here for testing SFINAE.

150

Ack. Ill rewrite these tests so they check situations the STL is not allowed to strengthen.

test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp
66

I thought I regex replaced them all. I guess not :-(

test/std/utilities/variant/variant.variant/variant.ctor/in_place_index_init_list_Args.pass.cpp
1 ↗(On Diff #78710)

Doesn't really matter. Making them all lower case for consistency.

test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp
545

Ack.

EricWF updated this revision to Diff 78975.Nov 22 2016, 4:37 PM
EricWF edited edge metadata.
EricWF marked 7 inline comments as done.

Update with fixes.

STL_MSFT added inline comments.Nov 22 2016, 4:41 PM
test/std/utilities/variant/variant.helpers/variant_alternative.pass.cpp
34

Of course <variant> drags in <type_traits>, I'm just being nitpicky :-)

CaseyCarter added a comment.EditedNov 22 2016, 4:55 PM

Other than the note in variant.variant\variant.swap\swap.pass.cpp, all passes now pass and all fails now fail with our implementation.

test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp
488

This instantiates variant<int, NotSwappable>::swap without meeting the requirements thereof (i.e., our member swap does not SFINAE).

CaseyCarter added inline comments.Nov 22 2016, 4:57 PM
test/std/utilities/variant/variant.helpers/variant_alternative.pass.cpp
34

STL's internal compiler is well-known for being more strict than any shipping compiler.

EricWF marked an inline comment as done.Nov 22 2016, 5:03 PM
EricWF added inline comments.
test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp
488

Forgot to wrap that in a LIBCPP_ONLY.

EricWF updated this revision to Diff 79001.Nov 22 2016, 5:03 PM
EricWF marked an inline comment as done.

Wrap explicit instantiation in LIBCPP_ONLY block.

EricWF accepted this revision.Nov 22 2016, 5:11 PM
EricWF added a reviewer: EricWF.

Checking these into libc++ shortly. (But I'm disabling them for libc++ for obvious reasons).

This revision is now accepted and ready to land.Nov 22 2016, 5:11 PM
EricWF closed this revision.Nov 22 2016, 5:14 PM

r287728.