Page MenuHomePhabricator

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

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

Details

Summary

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

Notes:
__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.

libcxx/include/variant
331

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

1669–1672

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?

libcxx/test/std/utilities/variant/variant.relops/relops.pass.cpp
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.

libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
367

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.

379

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.
libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
379

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.
libcxx/include/variant
1669–1672

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.

libcxx/test/std/utilities/variant/variant.relops/relops.pass.cpp
274 ↗(On Diff #326316)

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

libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
367

Will do.

379

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
libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
379

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.
libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
376

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.

libcxx/utils/generate_feature_test_macro_components.py
630

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
libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
376

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

libcxx/utils/generate_feature_test_macro_components.py
630

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.
libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
376

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

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'
#define _VSTD std::_LIBCPP_ABI_NAMESPACE
              ^
/home/marek/llvm-project/libcxx/include/variant:1680:10: note: in instantiation of function template specialization 'std::__throw_if_valueless<BadVariant>' requested here
  _VSTD::__throw_if_valueless(_VSTD::forward<_Vs>(__vs)...);
         ^
/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.

libcxx/utils/generate_feature_test_macro_components.py
630

@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
libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
376

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

curdeius added inline comments.Mar 4 2021, 12:54 PM
libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
372

s/pick/picks/.
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.
libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
376

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

ldionne added inline comments.Mar 4 2021, 3:36 PM
libcxx/utils/generate_feature_test_macro_components.py
630

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
libcxx/test/std/utilities/variant/variant.relops/relops.pass.cpp
293 ↗(On Diff #329585)

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

libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
357

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?

391

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

408

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

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

409

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
libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
357

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

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.

libcxx/include/variant
340

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

libcxx/test/std/language.support/support.limits/support.limits.general/version.version.pass.cpp
4059

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

libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
357

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

421

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
libcxx/include/variant
340

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

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

LGTM with minor nitpick. Thanks!

libcxx/include/variant
1680

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
libcxx/include/variant
1680

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

Ship it!

libcxx/include/variant
1680

My bad, I looked too quickly.