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

[Eric Fiselier via cfe-commits]

EricWF marked 45 inline comments as done.
EricWF added a comment.

Address almost all inline comments.



================
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);
----------------
STL_MSFT wrote:
> Technically, addressof() lives in <memory>.
I'm just changing it to `&v`.


================
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>, "");
----------------
STL_MSFT wrote:
> You aren't directly including <type_traits> (not sure if "variant_test_helpers.hpp" is).
Fixing this. Does your `<variant>` somehow not drag in `<type_traits>`?


================
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;
----------------
STL_MSFT wrote:
> NoCopy doesn't even have a default ctor, how are you supposed to make one? Appears to apply to the classes below.
Don't care about making them. They are only here for testing SFINAE.


================
Comment at: test/std/utilities/variant/variant.variant/variant.assign/copy.pass.cpp:149
+        static_assert(!std::is_nothrow_copy_assignable<V>::value, "");
+    }
+}
----------------
CaseyCarter wrote:
> 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.
Ack. Ill rewrite these tests so they check situations the STL is not allowed to strengthen.


================
Comment at: test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp:65
+    }
+#if defined(VARIANT_TEST_REFERENCES)
+    {
----------------
STL_MSFT wrote:
> You have like twenty different macros for enabling references.
I thought I regex replaced them all. I guess not :-(


================
Comment at: test/std/utilities/variant/variant.variant/variant.ctor/in_place_index_init_list_Args.pass.cpp:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
STL_MSFT wrote:
> Should this test filename contain capital A?
Doesn't really matter. Making them all lower case for consistency.


================
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>, "");
----------------
CaseyCarter wrote:
> This should be `LIBCPP_STATIC_ASSERT` (SFINAE isn't mandated for member swap after LWG2749).
Ack.


https://reviews.llvm.org/D26903