[libcxx-commits] [PATCH] D132327: [libc++] Implement P2445R1 (`std::forward_like`)

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__utility/forward_like.h:25
+
+template <class _Tp, class _Utp>
+[[nodiscard]] constexpr auto&& forward_like(_Utp&& __ux) noexcept {
----------------
Or is there a reason for the `t`?


================
Comment at: libcxx/include/__utility/forward_like.h:26
+template <class _Tp, class _Utp>
+[[nodiscard]] constexpr auto&& forward_like(_Utp&& __ux) noexcept {
+  static_assert(__can_reference<_Tp>, "std::forward_like's first template argument must be a referenceable type.");
----------------
personally I'd use `decltype(auto)` instead. Not sure what the consensus on that is though.


================
Comment at: libcxx/include/__utility/forward_like.h:28
+  static_assert(__can_reference<_Tp>, "std::forward_like's first template argument must be a referenceable type.");
+
+  using __unrefT = remove_reference_t<_Tp>;
----------------
Is there a reason you don't want to use aliases and conditionals like the standard does? The current implementation is very hard to compare to the standard.

Something like
```
template <class _Ap, class _Bp>
using _CopyConst = _If<is_const_v<_Ap>, const _Bp, _Bp>;

template <class _Ap, class _Bp>
using _OverrideRef = _If<is_rvalue_reference_v<_Ap>, remove_reference_t<_Bp>&&, _Bp&>;

using _Vp = _OverrideRef<_Tp&&, _CopyConst<remove_reference_t<_Tp>, remove_reference_t<_Up>>>;

return static_cast<_Vp>(__ux);
```
would be a lot clearer IMO.



================
Comment at: libcxx/include/utility:45-60
+template <typename T, typename U>
+using __override_ref_t = std::conditional_t<std::is_rvalue_reference_v<T>,
+                                            std::remove_reference_t<U> &&, U &>;
+template <typename T, typename U>
+using __copy_const_t =
+    std::conditional_t<std::is_const_v<std::remove_reference_t<T>>,
+                       U const, U>;
----------------
Why do you have (AFAICT non-existent) implementation details in the synopsis? You are also missing a `// since C++23` after `forwrward_like`.


================
Comment at: libcxx/include/utility:57-59
+template <typename T>
+[[nodiscard]] constexpr
+auto forward_like(auto&& x) noexcept -> __forward_like_t<T, decltype(x)>;
----------------
This should be copied from the standard.


================
Comment at: libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.compile.pass.cpp:1-12
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
----------------
Two license headers seem weird. Are you sure the LLVM license header should be here?


================
Comment at: libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.compile.pass.cpp:13
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include <cassert>
----------------
I'm missing tests that ensure `forward_like` doesn't copy/move anything around or requires any constructors. A test that forwards the same type would also be nice. You should also check that `forward_like` is `noexcept`.


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

https://reviews.llvm.org/D132327