[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 a comment.

In D92044#2491583 <https://reviews.llvm.org/D92044#2491583>, @ldionne wrote:

> @rarutyun Can you please rebase onto `main` so that the CI will trigger? I added the LLVM monorepo tag to the review.

@ldionne, sure, done.



================
Comment at: libcxx/include/variant:631
+      if constexpr (is_void_v<_Rp>) {
+        _VSTD::__invoke_constexpr(_VSTD::forward<_Visitor>(__visitor),
+                                  _VSTD::forward<_Alts>(__alts).__value...);
----------------
zoecarver wrote:
> Nit: these can simply be `invoke` now that D93815 landed.
Ok, thanks. I would prefer to leave it as is because we have lots of `__invoke_constexpr` usage in the code. If we want to refactor that file I believe it should be the separate patch.


================
Comment at: libcxx/test/std/utilities/variant/variant.visit/visit_return_type.pass.cpp:335
+      return true;
+    } catch (...) {
+    }
----------------
zoecarver wrote:
> Is there a chance this will throw anything other than bad variant access?
I don't think so for now and I believe this test makes sure that only `std::bad_variant_access` can be thrown. I cannot say for C++ future, of course:) But that's the state of the art we have.

I just wanted to make sure that `std::visit` with explicit return type has non less test set than regular `std::visit` does where applicable.  


================
Comment at: libcxx/test/support/variant_test_helpers.h:15
 #include <cassert>
+#include <variant>
 
----------------
zoecarver wrote:
> Is this header needed?
Thanks for noticing. Fixed. This header **was** needed unless I moved specific tests for `visit` and `visit<R>`.


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

https://reviews.llvm.org/D92044