This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Diff Detail

Event Timeline

fsb4000 created this revision.Aug 21 2022, 8:25 AM
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
25

Or is there a reason for the t?

26

personally I'd use decltype(auto) instead. Not sure what the consensus on that is though.

28

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

Two license headers seem weird. Are you sure the LLVM license header should be here?

13

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
73–83

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
25

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
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
12 ↗(On Diff #456106)

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
12 ↗(On Diff #456106)

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
8 ↗(On Diff #456106)

I guess this is inherited from the MSVC testing style?

67 ↗(On Diff #456106)

Are these changes in the upstream MSVC test? If not, I'd rather have them in a separate test to reduce divergence from upstream.

73 ↗(On Diff #456106)

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
12 ↗(On Diff #456106)

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
8 ↗(On Diff #456106)

Yes, MSVC using this style.

67 ↗(On Diff #456106)

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.

73 ↗(On Diff #456106)

Ok, I will not use the alias.

libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.pass.cpp
12 ↗(On Diff #456106)

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
12 ↗(On Diff #456106)

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.