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

[curdeius]

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

• https://wg21.link/P2162

[BRevzin]

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]

• Fix gcc build.
• Add test for visiting a non-evil derived variant.

[ldionne]

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

[tcanens]

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

379

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

[curdeius]

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]

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]

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

634

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]

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

634

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

[curdeius]

• 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

634

@ldionne, what's your opinion on this?

[tcanens]

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

376

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

[curdeius]

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]

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

376

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

[ldionne]

libcxx/utils/generate_feature_test_macro_components.py

634

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]

• Make visit SFINAE-friendly.
• Make visit<R> SFINAE-friendly.
• Bump feature-test macro for C++17 onwards.

[curdeius]

• Fix csv status.
• Fmt.

[• Quuxplusone]

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]

• Remove relops changes.
• Add has_visit tests.
• Add test for get<Index>.

[curdeius]

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]

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

4107

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]

• Change order of __as_variant overloads.
• Test &, const &, &&, const &&.
• Fmt.
• Rebase.

[curdeius]

libcxx/include/variant

340

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

[ldionne]

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.

[curdeius]

libcxx/include/variant

1680

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

[ldionne]

Ship it!

libcxx/include/variant

1680

My bad, I looked too quickly.