Or is there a reason for the t?
personally I'd use decltype(auto) instead. Not sure what the consensus on that is though.
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.
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.
Why do you have (AFAICT non-existent) implementation details in the synopsis? You are also missing a // since C++23 after forwrward_like.
This should be copied from the standard.
|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.
The standard spec has a trailing return type, meaning if the return type is ill formed, the function should be SFINAE out.
|72–82 ↗||(On Diff #454319)|
I think it should also run the test at runtime and the test() should also test all different combinations.
Originally they were _Ty and _Uty but I noticed that libc++ uses _Tp so I renamed y to p.
Yes, I will add // since C++23 and I look at the standard.
|1–12 ↗||(On Diff #454314)|
Maybe you're right.
So probably I should do the same.
I think the return type is just _ForwardLike<_Tp, _Up>
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
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
[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
I guess this is inherited from the MSVC testing style?
Are these changes in the upstream MSVC test? If not, I'd rather have them in a separate test to reduce divergence from upstream.
Why do you use an alias here? IMO it makes the test really hard to read.
This way to include the test seems quite weird to me. Why can't we just name the MSVC test test.pass.cpp?
Yes, MSVC using this style.
I will create a PR to the MSVC test after this patch will be landed.
Ok, I will not use the alias.
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.
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.