[PATCH] D26903: [libcxx] Add <variant> tests (but not implementation)

[Casey Carter via cfe-commits]

CaseyCarter added inline comments.


================
Comment at: test/std/utilities/variant/variant.relops/relops.pass.cpp:60
+inline bool operator>(MakeEmptyT const&, MakeEmptyT const&)  { assert(false); }
+inline bool operator>=(MakeEmptyT const&, MakeEmptyT const&) { assert(false); }
+
----------------
MSVC with /W4 is, uh, not found of these functions. Adding "return false;" after the assert shuts it up.


================
Comment at: test/std/utilities/variant/variant.relops/relops.pass.cpp:189
+        V v2; makeEmpty(v2);
+        assert(test_less(v1, v2, true, false));
+    }
----------------
Bogus test: should be `assert(test_less(v1, v2, false, true));`. Maybe someone forgot to implement part of P0032?


================
Comment at: test/std/utilities/variant/variant.relops/relops.pass.cpp:195
+        V v2;
+        assert(test_less(v1, v2, false, true));
+    }
----------------
Bogus test again: should be `assert(test_less(v1, v2, true, false));`.


================
Comment at: test/std/utilities/variant/variant.variant/variant.assign/copy.pass.cpp:149
+        static_assert(!std::is_nothrow_copy_assignable<V>::value, "");
+    }
+}
----------------
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.


================
Comment at: test/std/utilities/variant/variant.variant/variant.mod/emplace_index_args.pass.cpp:92
+        int y = 42;
+        int z = 43;
+        V v(std::in_place_index<0>, -1);
----------------
y and z are unused here,


================
Comment at: test/std/utilities/variant/variant.variant/variant.mod/emplace_type_args.pass.cpp:93
+        int y = 42;
+        int z = 43;
+        V v(std::in_place_type<int>, -1);
----------------
y and z are unused here.


================
Comment at: test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp:321
+        // the variant swap is called on.
+        assert(T::move_called == 1);
+        assert(T::move_assign_called == 0);
----------------
Non-portable. Replace with `LIBCPP_ASSERT(T::move_called == 1); assert(T::move_called <= 2);`


================
Comment at: test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp:328
+        assert(T::swap_called == 0);
+        assert(T::move_called == 2);
+        assert(T::move_assign_called == 0);
----------------
Non-portable. Replace with `LIBCPP_ASSERT(T::move_called == 2); assert(T::move_called <= 2);`


================
Comment at: test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp:349
+        assert(T1::move_called == 1); // throws
+        assert(T1::move_assign_called  == 0);
+        assert(T2::move_called == 1);
----------------
Extra space before `==` here and on 370.


================
Comment at: test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp:350
+        assert(T1::move_assign_called  == 0);
+        assert(T2::move_called == 1);
+        assert(std::get<0>(v1).value == 42);
----------------
Non-portable. Replace with `LIBCPP_ASSERT(T2::move_called == 1); assert(T2::move_called <= 1);`


================
Comment at: test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp:352
+        assert(std::get<0>(v1).value == 42);
+        assert(v2.valueless_by_exception());
+    }
----------------
non-portable. s/b
```
if (T2::move_called != 0)
    assert(v2.valueless_by_exception());
else
    assert(std::get<1>(v2) == 100);
```



================
Comment at: test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp:367
+        }
+        assert(T2::move_called == 1);
+        assert(T2::swap_called == 0);
----------------
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);`


================
Comment at: test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp:371
+        assert(T2::move_assign_called  == 0);
+        assert(std::get<0>(v1).value == 42);
+        assert(std::get<1>(v2).value == 100);
----------------
Non-portable. S/b:
```
if (T1::move_called != 0)
    assert(v1.valueless_by_exception());
else
    assert(std::get<0>(v1).value == 42);
```


================
Comment at: test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp:375
+    {
+        using T1 = ThrowsOnSecondMove;
+        using T2 = NonThrowingNonNoexceptType;
----------------
This test and the next are non-portable and I can't be bothered to fix them.


================
Comment at: test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp:403
+    {
+        // testing libc++ extension. If either variant stores a nothrow move
+        // constructible type v1.swap(v2) provides the strong exception safety
----------------
I shouldn't have to point out that a test with the comment "testing libc++ extension" is non-portable ;)


================
Comment at: test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp:405
+        // constructible type v1.swap(v2) provides the strong exception safety
+        // guarentee.
+        using T1 = ThrowingTypeWithNothrowSwap;
----------------
guarantee


================
Comment at: test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp:544
+        using V = std::variant<int, NotSwappable>;
+        static_assert(!has_swap_member<V>());
+        static_assert(std::is_swappable_v<V>, "");
----------------
This should be `LIBCPP_STATIC_ASSERT` (SFINAE isn't mandated for member swap after LWG2749).


================
Comment at: test/std/utilities/variant/variant.visit/visit.pass.cpp:42
+  template <class ...Args>
+  bool operator()(Args&&... args) & {
+      set_call<Args&&...>(CT_NonConst | CT_LValue);
----------------
MSVC complains that all four of these operator() overloads name but do not use `args`; removing the name shuts up the warning.


https://reviews.llvm.org/D26903