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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a reviewer: mpark.
ldionne added a comment.
This revision now requires changes to proceed.

> Previous standards didn't disallow this AFAIK. But I'm OK to make this change C++2b-only.

I think it's fine to backport this to all versions of C++. If someone was taking advantage of this underspecification before C++20, we're only doing them a service to break their code sooner in previous C++ versions (that way they won't break when they upgrade).

> What else should be tested?

We don't have a test that `std::visit` with something that isn't derived from a `std::variant` doesn't work in a SFINAE-friendly manner.  Basically, testing this part of the paper: `+ Constraints: Vi is a valid type for all 0 <= i < n.`

> Is there a way to avoid __as_variant? Should it be a goal?

I don't think this should be a goal. The Standard describes it that way, I think it's fine.

This looks pretty good to me except for the comments I left. I would love for @mpark to take a look - his opinion overrides mine everywhere since he's the variant grand master.



================
Comment at: libcxx/include/variant:331
+template <class... _Types>
+inline _LIBCPP_INLINE_VISIBILITY constexpr const variant<_Types...>&
+__as_variant(const variant<_Types...>& __vs) noexcept {
----------------
No need for `inline` on these, they are already `inline` by way of being templates.


================
Comment at: libcxx/include/variant:1676
 constexpr void __throw_if_valueless(_Vs&&... __vs) {
-  const bool __valueless = (... || __vs.valueless_by_exception());
+  const bool __valueless = (... || _VSTD::__valueless_by_exception(__vs));
   if (__valueless) {
----------------
Calling `__as_variant(__vs).valueless_by_exception()` here instead would be closer to the Standard and would allow getting rid of `__valueless_by_exception` -- WDYT?


================
Comment at: libcxx/test/std/utilities/variant/variant.relops/relops.pass.cpp:274
+void test_comparisons() {
+  using xEq [[maybe_unused]] =
+      decltype(std::declval<Variant>() == std::declval<Variant>());
----------------
Do we really want to add `[[maybe_unused]]` to all typedefs in the test suite? I think we should just disable the unused typedef warning in the test suite if that's causing problems, that'll be much lighter notation-wise.


================
Comment at: libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:367
+
+  struct EvilVariant1 : std::variant<int, long, double>,
+                        std::tuple<int>,
----------------
Maybe add a short comment explaining what we expect and why it's useful to test this? I like to put myself in the shoes of my future self when I don't have the paper in front of me - I wouldn't understand why this test exists. If nothing else, perhaps mentioning the paper number is sufficient.


================
Comment at: libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:379
+  using xT [[maybe_unused]] = decltype(
+      std::visit(visitor_of_everything{}, std::declval<EvilVariant2>()));
+  using xTref [[maybe_unused]] = decltype(
----------------
Is there a reason why you're only calling those in an unevaluated context? Can't we actually call and "run" them (even though I understand it's trivial)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97394