[libcxx-commits] [PATCH] D92044: Implement P0655R1 visit<R>: Explicit Return Type for visit

[Michael Park via Phabricator via libcxx-commits]

mpark added inline comments.


================
Comment at: libcxx/include/variant:1637
+constexpr void __throw_if_valueless(_Visitor&& __visitor, _Vs&&... __vs) {
+  const bool __valueless = (... || __vs.valueless_by_exception());
+  if (__valueless) {
----------------
When we initially implemented this, there were shaky C++17 support for some compilers. It might be fine now... I guess we'll see if any of the build configs break.


================
Comment at: libcxx/include/variant:1662
+  return __variant::__visit_value<_Rp>(_VSTD::forward<_Visitor>(__visitor),
+                                  _VSTD::forward<_Vs>(__vs)...);
+}
----------------
nit: formatting


================
Comment at: libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:347
   test_exceptions();
-  test_caller_accepts_nonconst();
+  test_caller_accepts_nonconst<void>();
 
----------------
This is now testing `visit<void>(...)` rather than `visit(...)`, right? Don't we want to continue to test `visit(...);` here?


================
Comment at: libcxx/test/std/utilities/variant/variant.visit/visit_return_type.pass.cpp:246
+    constexpr V v(42);
+    static_assert((std::visit<void>(obj, v), 42) == 42, "");
+  }
----------------
rarutyun wrote:
> I decided to check the `constexpr` functionality for `void` return type with the `operator comma`. Please let me know if it makes sense from your perspecitve to test `constexpr`-ness of the function that returns `void`. To me it doesn't have much sense but I have done in case it's necessary.
I agree with you that it doesn't really make much sense. But these tests didn't exist, so I'm not really following why you felt the need to add them if it doesn't make sense to you. Is the intention that constexpr returning void should be tested? If so, why weren't they added to `visit.pass.cpp` as well...?

Anyway, I think to test constexpr void, we should be testing that we can perform mutating operations within a constexpr function, using a constexpr void function. Something like:

```
auto test = []() constexpr {
  std::variant<int> v = 101;
  std::visit<void>([](int& x) constexpr { x = 202; }, v);
  return get<int>(v) == 202;
};
static_assert(test());
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92044