This is an archive of the discontinued LLVM Phabricator instance.

[libc++][PSTL] Implement std::move
ClosedPublic

Authored by philnik on Jul 14 2023, 12:24 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGd2a46e6480f3: [libc++][PSTL] Implement std::move

Diff Detail

Event Timeline

philnik created this revision.Jul 14 2023, 12:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2023, 12:24 PM
philnik requested review of this revision.Jul 14 2023, 12:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2023, 12:24 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 546908.Aug 3 2023, 9:38 AM

Try to fix CI

ldionne added inline comments.
libcxx/include/__algorithm/pstl_move.h
32
43

This needs to be updated for exception safety, including tests.

libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/pstl.copy.pass.cpp
11

You have two tests that still use // REQUIRES: with-pstl, which means they are always disabled. Please fix in a separate patch.

ldionne added inline comments.Aug 14 2023, 8:35 AM
libcxx/include/__algorithm/pstl_move.h
1

__pstl_move is missing from the documentation in pstl_backend.h

philnik updated this revision to Diff 550008.Aug 14 2023, 10:09 AM
philnik marked 4 inline comments as done.

Address comments

ldionne added inline comments.Aug 14 2023, 10:45 AM
libcxx/include/__algorithm/pstl_move.h
23

Missing <optional> include.

libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.exception_handling.pass.cpp
1 ↗(On Diff #550008)

You need to add a test that __move is noexcept, we said we would add such tests in the PSTL / exception safety patch.

22–26 ↗(On Diff #550008)

This is never used.

libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.move.pass.cpp
82–85

This is never used.

philnik updated this revision to Diff 550488.Aug 15 2023, 2:52 PM
philnik marked 3 inline comments as done.

Address comments

libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.exception_handling.pass.cpp
1 ↗(On Diff #550008)

I thought we didn't need the test after all, since noexcept isn't necessary on the backend anymore and the death tests caught missing noexcepts quite well (and a std tests seems generally preferable to libc++-specific ones).

ldionne accepted this revision.Aug 16 2023, 8:30 AM

LGTM w/ comments applied.

libcxx/include/__algorithm/pstl_move.h
55

Right now, we perform a move-construction + an assignment for every element (https://godbolt.org/z/vTs9ofrrT).

This can be fixed with -> decltype(auto) on the lambda (https://godbolt.org/z/WKeMdEc3W)

Let's fix this and add a test. This is an important property of std::move!

libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.exception_handling.pass.cpp
1 ↗(On Diff #550008)

I think you're right, I just got confused with all the recent discussions we had about exception safety in PSTL.

libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.move.pass.cpp
1

Can you add tests with move-only types? This should definitely work since it's std::move.

Can you also take a look at the serial std::move tests to make sure we test it there? We should, but just to be certain.

This revision is now accepted and ready to land.Aug 16 2023, 8:30 AM
philnik updated this revision to Diff 557726.Oct 17 2023, 3:40 AM

Try to fix CI

This revision was landed with ongoing or failed builds.Oct 22 2023, 1:25 AM
This revision was automatically updated to reflect the committed changes.