Page MenuHomePhabricator

[libc++] [C++2b] [P2162] Allow inheritance from std::variant.

Authored by curdeius on Feb 24 2021, 8:26 AM.



This patch changes the variant even in pre-C++2b.
It should not break anything, only allow use cases that didn't work previously.

__as_variant is used in __visitation::__variant::__visit_alt, but I haven't used it in __visitation::__variant::__visit_alt_at.
That's because it is used only in __visit_value_at, which in turn is always used on variant specializations (that's in comparison operators).

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
curdeius requested review of this revision.Feb 24 2021, 8:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2021, 8:26 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
curdeius edited the summary of this revision. (Show Details)Feb 24 2021, 8:28 AM
curdeius added a reviewer: BRevzin.
BRevzin added a subscriber: mpark.Feb 24 2021, 8:47 AM

Previous standards didn't disallow this AFAIK.

I would describe it more as the previous standard underspecified whether this was allowed or not. libc++ already supported the use-cases that this paper was trying to standardize supporting.

This looks good to me but @mpark's opinion is more relevant here.

curdeius updated this revision to Diff 326316.Feb 25 2021, 1:03 AM
  • Fix gcc build.
  • Add test for visiting a non-evil derived variant.
ldionne requested changes to this revision.Mar 2 2021, 1:11 PM
ldionne added a reviewer: mpark.

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.


No need for inline on these, they are already inline by way of being templates.


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?

274 ↗(On Diff #326316)

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.


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.


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)?

This revision now requires changes to proceed.Mar 2 2021, 1:11 PM
tcanens added a subscriber: tcanens.Mar 2 2021, 9:35 PM
tcanens added inline comments.

Constructing the type_info base class might be a bit tricky...

curdeius planned changes to this revision.Mar 3 2021, 12:28 AM
curdeius added inline comments.

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.

274 ↗(On Diff #326316)

Indeed, it will be cleaner just do ignore this particular warning in this test.


Will do.


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).

ldionne added inline comments.Mar 3 2021, 4:28 PM

Oh, I hadn't thought of that. Yes, that's obvious now :-).

Feel free to disregard this comment and leave it unevaluated, or to do whatever you were thinking about with a hand-made class.

Quuxplusone added inline comments.

Would it be appropriate to also test the behavior of struct X : std::variant<int>, std::variant<long> {} and struct Y : private std::variant<int> {}? I don't think we expect either of those to support std::visit, but I wonder if they're supposed to SFINAE away cleanly or give some awful template error message or what.


Since you're making the change in all language versions (IIUC), shouldn't the feature-test macro have the higher value in all language versions? (Has this situation ever come up in WG21 before? what did the Committee think vendors should do?)

curdeius added inline comments.Mar 4 2021, 7:45 AM

I don't see how it may be SFINAE-friendly (but I haven't checked).


Ha, good catch. No idea what should be done, probably always setting to the higher value is OK.

curdeius updated this revision to Diff 328279.Mar 4 2021, 12:51 PM
curdeius marked 8 inline comments as done.
  • Use as_variant instead of valueless_by_exception.
  • Add tests without type_info.
  • Remove inline.

Well, I've checked those and they fail with more or less cryptic messages, but still I find them pretty nice.

struct BadVariant : std::variant<int>, std::variant<double> {
  using std::variant<int>::variant;
std::visit([](auto x) { assert(x == 12); }, BadVariant{12});

fails with:

/home/marek/llvm-project/libcxx/include/variant:1669:36: error: no matching function for call to '__as_variant'
  const bool __valueless = (... || _VSTD::__as_variant(__vs).valueless_by_exception());
/home/marek/llvm-project/libcxx/include/__config:779:15: note: expanded from macro '_VSTD'
/home/marek/llvm-project/libcxx/include/variant:1680:10: note: in instantiation of function template specialization 'std::__throw_if_valueless<BadVariant>' requested here
/home/marek/llvm-project/libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:397:8: note: in instantiation of function template specialization 'std::visit<(lambda at /home/marek/llvm-project/libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:397:14), BadVariant>' requested here
  std::visit([](auto x) { assert(x == 12); }, BadVariant{12});
/home/marek/llvm-project/libcxx/include/variant:326:1: note: candidate template ignored: failed template argument deduction
__as_variant(variant<_Types...>& __vs) noexcept {
/home/marek/llvm-project/libcxx/include/variant:332:1: note: candidate template ignored: failed template argument deduction
__as_variant(const variant<_Types...>& __vs) noexcept {

I think there's nothing we can do about it.


@ldionne, what's your opinion on this?

curdeius edited the summary of this revision. (Show Details)Mar 4 2021, 12:52 PM
tcanens added inline comments.Mar 4 2021, 12:53 PM

The wording is pretty clear that this case needs to SFINAE.

curdeius added inline comments.Mar 4 2021, 12:54 PM

Will fix typos locally for the moment. Here and in relops.

curdeius planned changes to this revision.Mar 4 2021, 1:07 PM
curdeius added inline comments.

Right. I might need to test it in a different manner though.

ldionne added inline comments.Mar 4 2021, 3:36 PM

Yes, I think we need to always use the higher value. Otherwise, we're pretending that we implement a slightly different version of std::variant than we really are.

curdeius updated this revision to Diff 329566.Mar 10 2021, 1:11 AM
  • Make visit SFINAE-friendly.
  • Make visit<R> SFINAE-friendly.
  • Bump feature-test macro for C++17 onwards.
curdeius marked 3 inline comments as done.Mar 10 2021, 1:11 AM
curdeius edited the summary of this revision. (Show Details)
curdeius updated this revision to Diff 329585.Mar 10 2021, 2:48 AM
  • Fix csv status.
  • Fmt.
Quuxplusone added inline comments.Mar 10 2021, 2:20 PM
293 ↗(On Diff #329585)

Here you don't mean "visit", you mean "relops." Actually I think lines 289-320 in this file are unnecessary.


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?


Nit: mark this operator() as const, for good style.


Nit: > > should be >> since this test needn't run in C++03 mode.

Non-nit: Please write 0, not 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).

curdeius planned changes to this revision.Mar 11 2021, 12:36 AM
curdeius updated this revision to Diff 332916.Wed, Mar 24, 3:06 AM
curdeius marked 4 inline comments as done.
  • Remove relops changes.
  • Add has_visit tests.
  • Add test for get<Index>.
curdeius added inline comments.Wed, Mar 24, 3:08 AM

Not sure if I correctly understood what test you suggested. Could you have a look please?

Quuxplusone accepted this revision as: Quuxplusone.Wed, Mar 24, 4:53 PM

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.


Minor bikeshed: & const& const&& && is a weird order to put these four overloads in :)


Make sure to re-run the generator scripts before landing. This diff should go away, I think.


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. :)


nit: >> is OK here

curdeius edited the summary of this revision. (Show Details)Thu, Mar 25, 2:21 AM
curdeius updated this revision to Diff 333238.Thu, Mar 25, 2:41 AM
curdeius marked 5 inline comments as done.
  • Change order of __as_variant overloads.
  • Test &, const &, &&, const &&.
  • Fmt.
  • Rebase.
curdeius added inline comments.Thu, Mar 25, 2:45 AM

I changed to paper's order (&, const &, &&, const &&).

ldionne accepted this revision.Thu, Mar 25, 6:24 AM

LGTM with minor nitpick. Thanks!


void_t here is not necessary. Just typename = decltype(...) should be sufficient for SFINAE. Same below.

This revision is now accepted and ready to land.Thu, Mar 25, 6:24 AM
curdeius added inline comments.Thu, Mar 25, 9:32 AM

@ldionne, but... _Vs is a parameter pack. How can that be done without void_t?

Ship it!


My bad, I looked too quickly.