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

[Ruslan Arutyunyan via Phabricator via libcxx-commits]

rarutyun marked 3 inline comments as done.
rarutyun 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) {
----------------
mpark wrote:
> 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.
Yes, agree. I may return it back if we'll see some failures. I propose to leave it as is for now


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


================
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, "");
+  }
----------------
mpark wrote:
> 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());
> ```
Let me explain my thoughts. I doubt if the similar test should be added to just `visit` because `visit` always has the same return type as the visitor. For the `visit<R>`  user may explicitly speсify the return type including `void`. That means we have some kind of branch where the return value of visitor can be either converted to `R` or can be ignored. Despite that, the function has to be `constexpr`. I added such tests to cover both those branches even if `void` one doesn't have much sense in real code.

I also considered to create some wrapper function (similar to your code snippet) where I would use `constexpr visit<void>` but I decided to use the same tests `visit` has with `operator comma`. I don't see a big difference with what you suggest vs what I did because both tests check if the function can be used in `constexpr` context.  The only difference is your test is more close to the real code use-cases.

Overall: I may add the test you suggest but we may still have the tests I wrote (with or without applying the suggestion) if you agree with my argumentation. If you think that tests I wrote create more confusion than value they may be removed.


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

https://reviews.llvm.org/D92044