Details
- Reviewers
philnik huixie90 jdoerfert - Group Reviewers
Restricted Project - Commits
- rG3a49cffe3add: [libc++] Implement P2445R1 (`std::forward_like`)
Diff Detail
Event Timeline
| libcxx/include/__utility/forward_like.h | ||
|---|---|---|
| 26 | Or is there a reason for the t? | |
| 27 | personally I'd use decltype(auto) instead. Not sure what the consensus on that is though. | |
| 29 | 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. | |
| libcxx/include/utility | ||
| 45–60 | Why do you have (AFAICT non-existent) implementation details in the synopsis? You are also missing a // since C++23 after forwrward_like. | |
| 57–59 | This should be copied from the standard. | |
| libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.compile.pass.cpp | ||
| 1–12 ↗ | (On Diff #454314) | Two license headers seem weird. Are you sure the LLVM license header should be here? |
| 13 ↗ | (On Diff #454314) | 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. |
| libcxx/include/__utility/forward_like.h | ||
|---|---|---|
| 16 | Please use the granularized type_traits headers instead. | |
| libcxx/include/__utility/forward_like.h | ||
|---|---|---|
| 27–28 | The standard spec has a trailing return type, meaning if the return type is ill formed, the function should be SFINAE out. | |
| libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.compile.pass.cpp | ||
| 72–82 ↗ | (On Diff #454319) | I think it should also run the test at runtime and the test() should also test all different combinations. |
| libcxx/include/__utility/forward_like.h | ||
|---|---|---|
| 26 | Originally they were _Ty and _Uty but I noticed that libc++ uses _Tp so I renamed y to p. | |
| libcxx/include/utility | ||
| 45–60 | I copied from https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2445r1.pdf Yes, I will add // since C++23 and I look at the standard. | |
| libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.compile.pass.cpp | ||
| 1–12 ↗ | (On Diff #454314) | Maybe you're right. and So probably I should do the same. |
| libcxx/include/utility | ||
|---|---|---|
| 45–60 | C++ standard has the same implementation details. | |
| libcxx/include/__utility/forward_like.h | ||
|---|---|---|
| 38–39 | I think the return type is just _ForwardLike<_Tp, _Up> | |
| libcxx/include/utility | ||
| 45–59 | I think you don't need to put implementation details here. just copy whatever in the standard template<class T, class U> [[nodiscard]] constexpr auto forward_like(U&& x) noexcept -> see below; // since C++23 | |
I tried to fix "Generated output) failed"
and it seems ninja -C build libcxx-generate-files has completely different behavior on Windows and Linux 😿
What should I do?
LGTM with nits. I will leave the final approval to others who also commented on this patch.
| libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.pass.cpp | ||
|---|---|---|
| 13 | Is this test supposed to be running on gcc/clang? I am slightly confused because it is in "msvc" folder | |
| libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.pass.cpp | ||
|---|---|---|
| 13 | Yes, all configurations should be tested. I used https://github.com/llvm/llvm-project/tree/main/libcxx/test/std/utilities/charconv/charconv.msvc as a naming example. I opened gcc 12/ C++ latest log: https://buildkite.com/llvm-project/libcxx-ci/builds/13210#0182deee-3853-484d-a646-79afa1e27420 and I found: [K... cfg.in :: std/utilities/utility/forward_like/forward_like.msvc/test.pass.cpp [A[K 98% [32m[[1m=========================================================--(B[m[32m](B[m ETA: 00:00:04 and C++2b log: https://buildkite.com/llvm-project/libcxx-ci/builds/13210#0182deee-3841-41d9-8725-06f9a249e560 [K... cfg.in :: std/utilities/utility/forward_like/forward_like.msvc/test.pass.cpp [A[K 98% [32m[[1m=========================================================--(B[m[32m](B[m ETA: 00:00:08 | |
| libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.cpp | ||
|---|---|---|
| 9 | I guess this is inherited from the MSVC testing style? | |
| 68 | Are these changes in the upstream MSVC test? If not, I'd rather have them in a separate test to reduce divergence from upstream. | |
| 74 | Why do you use an alias here? IMO it makes the test really hard to read. | |
| libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.pass.cpp | ||
| 13 | This way to include the test seems quite weird to me. Why can't we just name the MSVC test test.pass.cpp? | |
| libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.cpp | ||
|---|---|---|
| 9 | Yes, MSVC using this style. | |
| 68 | I will create a PR to the MSVC test after this patch will be landed. | |
| 74 | Ok, I will not use the alias. | |
| libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.pass.cpp | ||
| 13 | Sure, we can. If we name the test test.pass.cpp should I add LLVM licence comment too? | |
I glossed over the code and don't see any obvious issues. However I didn't do a real review.
| libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.pass.cpp | ||
|---|---|---|
| 13 |
I agree it's weird but we used this method before and it minimized the differences between our version and upstream. I still want to look at a way to include more MSVC STL tests in our repository; however that is very low on my todo list. | |
I removed using NCCM = NoCtorCopyMove;
I will create a PR to the MSVC test after this patch will be landed so
And I didn't rename test.cpp to test.pass.cpp because we have 2 (or 3 if we count constexpr unique_ptr patch) such tests.
So maybe different patch for that if we decide that something like that would be better:
c++ // LLVM license some workarounds if needed // clang-format off // MSVC licence code from MSVC test file. // clang-format on
The change broke the CI. I see you added a fix in D131315.
Since it breaks the CI and it's trivial to fix next time please commit the fix directly without a review.
If the fix is non-trivial it's better to revert the patch, fix it, review if needed, and reland the patch.
I already fixed it. It seems the line-ending caused it, but that wasn't caught in the build started from Phabricator.
Please use the granularized type_traits headers instead.