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

[Ruslan Arutyunyan via Phabricator via libcxx-commits]

rarutyun marked 2 inline comments as done.
rarutyun added inline comments.


================
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:
> > 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.
> I think keeping these tests are fine, the reason for my suggestion was due to my experience in seeing at least one situation where Clang allowed `(non-constexpr, constexpr)` by completely ignoring the `non-constexpr` portion of the expression. The way I presented above just ensures not only that a `non-constexpr` can be mentioned within a constexpr context, but also that its side-effects are observed.
Ok. I returned from vacation:) and ready to finish this review.

I have added test you suggested with the slight modifications. I have also rebase the change with the latest `main` branch. 

@mpark, 

Could you please have a look? I guess all the comments should be addressed


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

https://reviews.llvm.org/D92044