This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] adds concept `std::move_constructible`
ClosedPublic

Authored by cjdb on Feb 7 2021, 6:05 PM.

Details

Summary

Implements parts of:

  • P0898R3 Standard Library Concepts
  • P1754 Rename concepts to standard_case for C++20, while we still can

Depends on D77961

Diff Detail

Event Timeline

cjdb requested review of this revision.Feb 7 2021, 6:05 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2021, 6:05 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 322017.Feb 7 2021, 7:04 PM
cjdb edited the summary of this revision. (Show Details)

updates commit message to indicate dependency

Do we need to do some tests on
[concept.moveconstructible]/1 http://eel.is/c++draft/concepts#concept.moveconstructible-1 ? Or does constructible_­from and convertible_­to guarantee these conditions hold?

libcxx/test/std/concepts/lang/moveconstructible.pass.cpp
7 ↗(On Diff #322017)

Since the tests only has static_asserts it could be made a compile-time test. That can be done by renaming the file to move_constructible.compile.pass.cpp

12 ↗(On Diff #322017)

This seems to be a copy-paste. move_constructible has only one template argument.

cjdb updated this revision to Diff 322282.Feb 8 2021, 9:42 PM
cjdb marked 2 inline comments as done.

applies @Mordante's suggestions

cjdb updated this revision to Diff 322283.Feb 8 2021, 9:43 PM

adds new line to EOF

cjdb added a comment.Feb 8 2021, 9:43 PM

Do we need to do some tests on
[concept.moveconstructible]/1 http://eel.is/c++draft/concepts#concept.moveconstructible-1 ? Or does constructible_­from and convertible_­to guarantee these conditions hold?

The compiler doesn't check the semantic requirements (right now), so there's nothing to test. If we decide to promote violating the semantic requirements from IFNDR to ill-formed, that's when adding the tests will be helpful (right now they'd likely get in the way).

Mordante accepted this revision.Feb 9 2021, 9:42 AM

Do we need to do some tests on
[concept.moveconstructible]/1 http://eel.is/c++draft/concepts#concept.moveconstructible-1 ? Or does constructible_­from and convertible_­to guarantee these conditions hold?

The compiler doesn't check the semantic requirements (right now), so there's nothing to test. If we decide to promote violating the semantic requirements from IFNDR to ill-formed, that's when adding the tests will be helpful (right now they'd likely get in the way).

SGTM. Just a bit surprised the Standard doesn't require more tests. Especially surprised by std::invocable and std::regular_invocable that they're the same. But I assume it has something to do with hard to validate.

No more comment, so LGTM.

ldionne accepted this revision.Feb 9 2021, 2:53 PM

LGTM after you've considered my comments. You can update the review if you want to re-trigger CI, but no need to wait for my approval. This is great, thanks!

libcxx/test/std/concepts/lang/moveconstructible.compile.pass.cpp
2

Should this test be named move_constructible.compile.pass.cpp instead?

22

You can move those out of main() since they are just static_asserts.

libcxx/test/std/concepts/lang/moveconstructible.h
9

Please use TEST_STD_CONCEPTS_LAND_MOVECONSTRUCTIBLE_H or something that more clearly encodes the path of the file. Nitpicky :-)

This revision is now accepted and ready to land.Feb 9 2021, 2:53 PM
ldionne added inline comments.Feb 9 2021, 2:55 PM
libcxx/test/std/concepts/lang/moveconstructible.compile.pass.cpp
2

Ahh, I see, you've named them after the section in the standard. LGTM

cjdb updated this revision to Diff 322572.Feb 9 2021, 7:38 PM
cjdb marked 3 inline comments as done.

applies @ldionne's feedback

cjdb marked an inline comment as done.Feb 9 2021, 11:29 PM
ldionne accepted this revision.Feb 10 2021, 8:46 AM
This revision was automatically updated to reflect the committed changes.