Implements parts of:
- P0898R3 Standard Library Concepts
- P1754 Rename concepts to standard_case for C++20, while we still can
Depends on D96660
Paths
| Differential D96742
[libcxx] adds concept `std::assignable_from` ClosedPublic Authored by cjdb on Feb 15 2021, 6:59 PM.
Details
Summary Implements parts of:
Depends on D96660
Diff Detail
Event Timeline
Comment Actions The implementation matches the spec closely. I didn't look at the tests in careful detail, however as a general high-level comment: is there value in testing these concepts on a few common types like std::string, std::vector, etc. just to make sure they behave as we'd expect them? Those are the first types that users are going to use concepts with, so I tend to see value in making sure they work as expected (not that I see any reason they wouldn't, but catching bad surprises is the whole point). Basically, it's as if we had a few compilable examples of how to use the Standard Library.
Comment Actions
Agreed. I'll add a few for vector<int> and string. Any other commonly used types that we should care about (e.g. map) or are vector and string "good enough"? Comment Actions
The more the better, IMO. map, deque, unique_ptr, variant (?), just a few of the common ones. I doubt it'll actually help us find any issues with the concepts themselves, but it could potentially find us issues with those types! Comment Actions
I think it's a great idea, and one I'm happy to take on; but if we do it right now, any latent bugs in other types that we uncover will become blockers for getting the rest of the concepts merged into libc++ (see my recent email for a potentially large blocker). I propose making a separate patch for each concept where we test all the frequently used standard types. Those patches can be indefinitely blocked, which isn't great, but at least it doesn't prevent libc++ from being C++20 feature-complete in a reasonable amount of time. To reiterate: I'm not shrugging off this work, but I'd like to defer it to after __cpp_lib_concepts is defined. If someone is motivated to pick it up before then, feel free to do so: I can confirm at least one major issue will fall out of std::ranges::swap.
cjdb added a child revision: D97162: [libcxx] adds std::ranges::swap, std::swappable, and std::swappable_with.Feb 21 2021, 1:29 PM Comment Actions
I decided it's better to do it here and now, so done. Comment Actions
When we encounter these issues, I think we can disable the failing tests with a comment saying "re-enable once <bad issue> is fixed". At least the structure of the tests will be there, so it'll be easy to come back and fix it. This revision is now accepted and ready to land.Mar 2 2021, 9:15 AM Comment Actions Pending CI, of course. I think CI might be failing because it's trying to apply your patch without applying the pre-requisites. Closed by commit rG647af31e7483: [libcxx] adds concept `std::assignable_from` (authored by cjdb). · Explain WhyMar 3 2021, 10:42 PM This revision was automatically updated to reflect the committed changes. Comment Actions LGTM mod nits. (Seems like too many tests. The code change itself looks like a correct transliteration of the paper standard.)
Revision Contents
Diff 328028 libcxx/include/concepts
libcxx/test/std/concepts/lang/assignable.compile.pass.cpp
|
Why do you deviate from the wording in the standard? _Rhs __rhs versus _Rhs&& __rhs.