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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius added inline comments.


================
Comment at: libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:376
+
+  struct EvilVariant2 : std::variant<int, long>, std::type_info {};
+
----------------
curdeius wrote:
> Quuxplusone wrote:
> > 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.
> I don't see how it may be SFINAE-friendly (but I haven't checked).
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.


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:631
     "name": "__cpp_lib_variant",
-    "values": { "c++17": 201606 },
+    "values": { "c++17": 201606, "c++2b": 202102 },
     "headers": ["variant"],
----------------
curdeius wrote:
> Quuxplusone wrote:
> > 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?)
> Ha, good catch. No idea what should be done, probably always setting to the higher value is OK.
@ldionne, what's your opinion on this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97394