[libcxx-commits] [PATCH] D97394: [libc++] [C++2b] [P2162] Allow inheritance from std::variant.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/utilities/variant/variant.relops/relops.pass.cpp:293
+
+  // Check that visit does not take index nor valueless_by_exception members from the base class.
+  struct EvilVariantBase {
----------------
Here you don't mean "visit", you mean "relops." Actually I think lines 289-320 in this file are unnecessary.


================
Comment at: libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:357
+
+  // Check that visit does not take index nor valueless_by_exception members from the base class.
+  struct EvilVariantBase {
----------------
>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?


================
Comment at: libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:391
+  template <typename T>
+  void operator()(const T&) {}
+};
----------------
Nit: mark this `operator()` as `const`, for good style.


================
Comment at: libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:408
+
+  static_assert(has_visit<std::variant<int> >(int()));
+  static_assert(!has_visit<BadVariant>(int()));
----------------
Nit: `> >` should be `>>` since this test needn't run in C++03 mode.

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


================
Comment at: libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:409
+  static_assert(has_visit<std::variant<int> >(int()));
+  static_assert(!has_visit<BadVariant>(int()));
+}
----------------
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`).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97394/new/

https://reviews.llvm.org/D97394