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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius planned changes to this revision.
curdeius added inline comments.


================
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) {
----------------
ldionne wrote:
> 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?
Yes, I agree. That's actually because I started by this part and only later added `__as_variant`. I'll use `__as_variant` and remove `__valueless_by_exception`.


================
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>());
----------------
ldionne wrote:
> 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.
Indeed, it will be cleaner just do ignore this particular warning in this test.


================
Comment at: libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:367
+
+  struct EvilVariant1 : std::variant<int, long, double>,
+                        std::tuple<int>,
----------------
ldionne wrote:
> 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.
Will do.


================
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(
----------------
tcanens wrote:
> ldionne wrote:
> > 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)?
> Constructing the `type_info` base class might be a bit tricky...
Indeed, that was the reason why I just used unevaluated context.
I'll try to do something else instead, because the reason for using `type_info` that both `variant` and `type_info` has `__impl` member that is ambiguous when calling `visit` (prior to this patch).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97394