Page MenuHomePhabricator

[libc++] Implement P2445R1 (`std::forward_like`)
ClosedPublic

Authored by fsb4000 on Aug 21 2022, 8:25 AM.

Details

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2022, 8:25 AM
fsb4000 requested review of this revision.Aug 21 2022, 8:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2022, 8:25 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
fsb4000 updated this revision to Diff 454314.Aug 21 2022, 8:40 AM

apply clang-format.patch and generated_output.patch

fsb4000 updated this revision to Diff 454317.Aug 21 2022, 8:50 AM

add missing - to ReleaseNotes.rst

philnik requested changes to this revision.Aug 21 2022, 9:17 AM
philnik added a subscriber: philnik.
philnik added inline comments.
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.

This revision now requires changes to proceed.Aug 21 2022, 9:17 AM
philnik added inline comments.Aug 21 2022, 9:18 AM
libcxx/include/__utility/forward_like.h
16

Please use the granularized type_traits headers instead.

fsb4000 updated this revision to Diff 454319.Aug 21 2022, 9:31 AM

fix tests

huixie90 requested changes to this revision.Aug 21 2022, 12:37 PM
huixie90 added a subscriber: huixie90.
huixie90 added inline comments.
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.
I think we should keep what is in the spec rather than hard error in the function body

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.

This revision now requires changes to proceed.Aug 21 2022, 12:37 PM
fsb4000 added inline comments.Aug 21 2022, 9:44 PM
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.
Sure, I can remove the t.

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)
fsb4000 updated this revision to Diff 454555.Aug 22 2022, 10:13 AM
fsb4000 marked 10 inline comments as done.

applied suggestions from @philnik and @huixie90 code reviews

libcxx/include/utility
45–60

C++ standard has the same implementation details.
https://eel.is/c++draft/forward#lib:forward_like

fsb4000 updated this revision to Diff 454562.Aug 22 2022, 10:34 AM

remove #include <__iterator/iterator_traits.h>

fsb4000 updated this revision to Diff 454663.Aug 22 2022, 5:26 PM

try fix tests

huixie90 added inline comments.Aug 23 2022, 11:54 PM
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
fsb4000 updated this revision to Diff 455614.Aug 25 2022, 9:20 AM

apply huixie90's code review suggestions.

fsb4000 marked 2 inline comments as done.Aug 25 2022, 9:21 AM
fsb4000 updated this revision to Diff 456088.Aug 27 2022, 12:05 AM

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?

fsb4000 updated this revision to Diff 456089.Aug 27 2022, 12:18 AM

git apply generated_output.patch

fsb4000 updated this revision to Diff 456090.Aug 27 2022, 12:27 AM

CRLF => LF

fsb4000 updated this revision to Diff 456092.Aug 27 2022, 1:14 AM

remove a blank space.

fsb4000 updated this revision to Diff 456106.Aug 27 2022, 3:49 AM

move => std::move

huixie90 accepted this revision as: huixie90.Aug 28 2022, 2:42 AM

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

fsb4000 marked an inline comment as done.Aug 28 2022, 4:31 AM
fsb4000 added inline comments.
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:

... cfg.in :: std/utilities/utility/forward_like/forward_like.msvc/test.pass.cpp
 98% [=========================================================--(B](B ETA: 00:00:04

and C++2b log: https://buildkite.com/llvm-project/libcxx-ci/builds/13210#0182deee-3841-41d9-8725-06f9a249e560

... cfg.in :: std/utilities/utility/forward_like/forward_like.msvc/test.pass.cpp
 98% [=========================================================--(B](B ETA: 00:00:08
philnik added inline comments.Aug 28 2022, 6:09 AM
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?

fsb4000 marked an inline comment as done.Aug 28 2022, 9:44 AM
fsb4000 added inline comments.
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.
I'm almost sure they will merge it without issues.

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.
I did similar to https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/utilities/charconv/charconv.msvc/test.pass.cpp

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

This way to include the test seems quite weird to me. Why can't we just name the MSVC test test.pass.cpp?

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.

fsb4000 updated this revision to Diff 456984.Aug 31 2022, 9:07 AM
fsb4000 marked 5 inline comments as done.

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
philnik accepted this revision.Sep 1 2022, 10:17 AM

LGTM.

This revision is now accepted and ready to land.Sep 1 2022, 10:17 AM
This revision was automatically updated to reflect the committed changes.

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.