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

[Stephan T. Lavavej via cfe-commits]

STL_MSFT added a comment.

Found some minor issues.



================
Comment at: test/std/utilities/variant/variant.get/get_if_index.pass.cpp:92
+        V v(42l);
+        ASSERT_SAME_TYPE(decltype(std::get_if<1>(std::addressof(v))), long*);
+        assert(*std::get_if<1>(std::addressof(v)) == 42);
----------------
Technically, addressof() lives in <memory>.


================
Comment at: test/std/utilities/variant/variant.get/get_if_type.pass.cpp:52
+        const V v(x);
+        ASSERT_SAME_TYPE(decltype(std::get_if<int&>(std::addressof(v))), int*);
+        assert(std::get_if<int&>(std::addressof(v)) == &x);
----------------
<memory> for addressof() (although I'll note that the other tests are saying &v).


================
Comment at: test/std/utilities/variant/variant.get/get_index.pass.cpp:176
+        ASSERT_SAME_TYPE(decltype(std::get<0>(std::move(v))), const int&&);
+        assert(std::get<0>(std::move(v)) == 42);
+    }
----------------
Technically, <utility> for move().


================
Comment at: test/std/utilities/variant/variant.get/get_index.pass.cpp:220
+template <std::size_t I>
+using Idx = std::integral_constant<size_t, I>;
+
----------------
<type_traits> for integral_constant.


================
Comment at: test/std/utilities/variant/variant.get/get_type.pass.cpp:99
+        int x = 42;
+        V v(std::move(x));
+        ASSERT_SAME_TYPE(decltype(std::get<int&&>(v)), int&);
----------------
<utility> move()


================
Comment at: test/std/utilities/variant/variant.hash/hash.pass.cpp:15
+
+// template <class ..._Types> struct hash<variant<Types...>>;
+// template <> struct hash<monostate>;
----------------
"<class ..._Types>" has really terrible spacing. (Or as I like to say, "Space. The final frontier.")


================
Comment at: test/std/utilities/variant/variant.helpers/variant_alternative.pass.cpp:33
+void test() {
+    static_assert(std::is_same_v<
+        typename std::variant_alternative<I, V>::type, E>, "");
----------------
You aren't directly including <type_traits> (not sure if "variant_test_helpers.hpp" is).


================
Comment at: test/std/utilities/variant/variant.helpers/variant_alternative.pass.cpp:60
+    }
+#if !defined(TEST_VARIANT_REFERENCES)
+    {
----------------
The sense of this #if test seems to be flipped.


================
Comment at: test/std/utilities/variant/variant.helpers/variant_size.pass.cpp:20
+// template <class T> constexpr size_t variant_size_v
+// = variant_size<T>::value;
+
----------------
No indentation! :-<


================
Comment at: test/std/utilities/variant/variant.helpers/variant_size.pass.cpp:35
+    static_assert(std::variant_size_v<const volatile V> == E, "");
+    static_assert(std::is_base_of<std::integral_constant<std::size_t, E>,
+                                  std::variant_size<V>>::value, "");
----------------
Need <type_traits>.


================
Comment at: test/std/utilities/variant/variant.helpers/variant_size.pass.cpp:41
+{
+    test<std::variant<>, 0>();
+    test<std::variant<void*>, 1>();
----------------
Hmm, is this cromulent now that empty variants are forbidden?


================
Comment at: test/std/utilities/variant/variant.monostate/monostate.pass.cpp:20
+#include <type_traits>
+#include <cassert>
+
----------------
Don't actually need <cassert> since you have no plain asserts and this is C++17.


================
Comment at: test/std/utilities/variant/variant.relops/relops.pass.cpp:59
+inline bool operator<=(MakeEmptyT const&, MakeEmptyT const&) { assert(false); }
+inline bool operator>(MakeEmptyT const&, MakeEmptyT const&)  { assert(false); }
+inline bool operator>=(MakeEmptyT const&, MakeEmptyT const&) { assert(false); }
----------------
No space after >, but space after < above. Madness!


================
Comment at: test/std/utilities/variant/variant.relops/relops.pass.cpp:66
+    try {
+        v = std::move(v2);
+        assert(false);
----------------
<utility> move()


================
Comment at: test/std/utilities/variant/variant.variant/variant.assign/copy.pass.cpp:28
+struct NoCopy {
+  NoCopy(NoCopy const&) = delete;
+  NoCopy& operator=(NoCopy const&) = default;
----------------
NoCopy doesn't even have a default ctor, how are you supposed to make one? Appears to apply to the classes below.


================
Comment at: test/std/utilities/variant/variant.variant/variant.assign/move.pass.cpp:114
+    {
+        // variant only provides move assignment when beth the move and move
+        // constructors are well formed
----------------
"beth". Also, "move and move constructors"?


================
Comment at: test/std/utilities/variant/variant.variant/variant.assign/move.pass.cpp:212
+        V v2(42);
+        V& vref = (v1 = std::move(v2));
+        assert(&vref == &v1);
----------------
<utility> move()


================
Comment at: test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp:65
+    }
+#if defined(VARIANT_TEST_REFERENCES)
+    {
----------------
You have like twenty different macros for enabling references.


================
Comment at: test/std/utilities/variant/variant.variant/variant.ctor/in_place_index_init_list_Args.pass.cpp:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
Should this test filename contain capital A?


================
Comment at: test/std/utilities/variant/variant.variant/variant.ctor/in_place_type_Args.pass.cpp:23
+#include <type_traits>
+#include <string>
+#include <cassert>
----------------
This file doesn't actually use <string>.


================
Comment at: test/std/utilities/variant/variant.variant/variant.ctor/move.pass.cpp:60
+  MakeEmptyT(MakeEmptyT &&) {
+    throw 42;
+  }
----------------
Inconsistent indentation.


================
Comment at: test/std/utilities/variant/variant.variant/variant.mod/emplace_index_args.pass.cpp:88
+    {
+        using V = std::variant<int, long, const void*,
+            TestTypes::NoCtors, std::string>;
----------------
Why wrap here?


================
Comment at: test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp:114
+    noexcept(NT_Swap) {
+    lhs.swap_called++;
+    do_throw<!NT_Swap>();
----------------
Postincrement? You monster!


================
Comment at: test/std/utilities/variant/variant.variant/variant.swap/swap.pass.cpp:286
+        assert(T::move_assign_called == 0);
+        assert(std::get<0>(v1).value == 42); // throw happend before v1 was moved from
+        assert(std::get<0>(v2).value == 100);
----------------
happend


================
Comment at: test/std/utilities/variant/variant.visit/visit.pass.cpp:71
+      last_call_type = type;
+      last_call_args = std::addressof(makeArgumentID<Args...>());
+  }
----------------
Technically, <memory> for addressof().


================
Comment at: test/std/utilities/variant/variant.visit/visit.pass.cpp:105
+        assert(Fn::check_call<int&>(CT_Const | CT_LValue));
+        std::visit(std::move(obj), v);
+        assert(Fn::check_call<int&>(CT_NonConst | CT_RValue));
----------------
<utility> move()


================
Comment at: test/std/utilities/variant/variant.visit/visit.pass.cpp:202
+struct ReturnFirst {
+  template <class ...Args>
+  constexpr int operator()(int f, Args&&...) const {
----------------
Space. The final frontier. (Occurs below)


================
Comment at: test/support/variant_test_helpers.hpp:53
+  MakeEmptyT(MakeEmptyT &&) {
+    throw 42;
+  }
----------------
Indentation


================
Comment at: test/support/variant_test_helpers.hpp:63
+};
+static_assert(std::is_swappable_v<MakeEmptyT>, ""); // required for test
+
----------------
<type_traits>


https://reviews.llvm.org/D26903