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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM!
My understanding is that it's still blocked on @ldionne to re-review and see if his (very long-ago) comments have been sufficiently addressed.



================
Comment at: libcxx/include/variant:340
+  return _VSTD::move(__vs);
+}
+
----------------
Minor bikeshed: `&` `const&` `const&&` `&&` is a weird order to put these four overloads in :)


================
Comment at: libcxx/test/std/language.support/support.limits/support.limits.general/version.version.pass.cpp:4107
 
-# if !_LIBCPP_HAS_NO_CONCEPTS
+# if defined(__cpp_concepts) && __cpp_concepts >= 201907L
 #   ifndef __cpp_lib_math_constants
----------------
Make sure to re-run the generator scripts before landing. This diff //should// go away, I think.


================
Comment at: libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:421
+  static_assert(!has_visit<BadVariant2>(0));
+  static_assert(has_visit<std::variant<int> >(0));
+  static_assert(has_visit<GoodVariant>(0));
----------------
nit: `>>` is OK here


================
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 {
----------------
curdeius wrote:
> Quuxplusone wrote:
> > 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?
> Not sure if I correctly understood what test you suggested. Could you have a look please?
Yes, that's what I meant. :) Except let's make one of those MyVariant arguments a const reference too, for extra overload-resolution matchy-matchiness.
```
const MyVariant v1 = 42;
std::visit([](auto x) { assert(x == 42); }, v1);
std::visit([](auto x) { assert(x == -1.25f); }, MyVariant(-1.25f));
```
Drive-by changed the float to a value that can be represented exactly in base-2, because I'm paranoid about floats. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97394