Implements parts of:
- P0898R3 Standard Library Concepts
- P1754 Rename concepts to standard_case for C++20, while we still can
Depends on D97359
Details
Details
- Reviewers
ldionne EricWF miscco Mordante curdeius • Quuxplusone zoecarver - Group Reviewers
Restricted Project - Commits
- rG8ef69c66d5aa: [libcxx] adds concept std::copyable
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| libcxx/test/std/concepts/object/copyable.compile.pass.cpp | ||
|---|---|---|
| 112 | I think this test can/should be a .verify.cpp test because there's not actually any runtime logic. | |
| 112 | I just saw this is marked as compile.pass, I suppose that does the same thing. | |
| libcxx/test/std/concepts/object/copyable.h | ||
| 1 ↗ | (On Diff #328730) | I think we generally want to put these in the support directory so they're not just floating around. But I don't feel strongly on this (if others disagree). |
Comment Actions
As usual, the code in <concepts> seems obviously correct, and the tests seem too verbose.
| libcxx/test/std/concepts/object/copyable.compile.pass.cpp | ||
|---|---|---|
| 34 | I see nothing testworthy about these four pointer types. The cv-qualification of the pointed-to type is obviously irrelevant to the copyability of the pointer itself. However, it might be interesting to test some top-level-cv-qualified class types, i.e. const S and volatile S, especially w.r.t. when they have (or don't have) const-qualified assignment operators. | |
| 63 | The cv-qualification of the pointed-to member function type is obviously irrelevant to the copyability of the pointer type itself. I'd cut these 25 lines down to 1. | |
| 112 | This is the first I've heard of these extensions, but I grepped and found utils/libcxx/test/format.py whose comments support the idea that .compile.pass.cpp is superior to .verify.cpp. :) | |
| libcxx/test/std/concepts/object/copyable.h | ||
| 1 ↗ | (On Diff #328730) | For these files that are included from only one .cpp file, personally I'd like to see them all inlined (and then also massively shortened). support/ should be for stuff that other people might one day want to reuse. These files are pretty clearly single-use. |
| 34 ↗ | (On Diff #328730) | The const keyword on a by-value function parameter is meaningless; please remove it. (And then rename to no_assignment.) |
| 41 ↗ | (On Diff #328730) | I see nothing testworthy about derived_from_noncopyable or has_noncopyable. |
| libcxx/test/std/concepts/object/copyable.compile.pass.cpp | ||
|---|---|---|
| 34 |
See below.
Huh, I only tested cv-qualified fundamental types. I'll get on some class types too. | |
| 63 | These are here to expose defects in the compiler, not the correctness of the library feature. Please see https://reviews.llvm.org/D74291#inline-674926 for context. | |
| libcxx/test/std/concepts/object/copyable.h | ||
| 1 ↗ | (On Diff #328730) |
The headers in libcxx/test/std/concepts/... were designed for testing the concept in the name of the file (and anything that refines them). Having said that, none of the files rely on concepts, so I don't have a problem with moving them if they'd help simplify some other tests.
The headers in this directory are also used by the concepts that refine the header's name (so movable.h is included here, copyable.h will be included in semiregular.h, and there won't be a need for a regular.h). I'd prefer to keep the headers to reduce the copy/paste in tests (it draws more attention to what has changed across the four concepts, since the tests are very similar). |
| 34 ↗ | (On Diff #328730) | This was supposed to test the requirement assignable_from<T&, const T>, but now that you point it out, I'm not sure how to test that in isolation of the other requirements. |
| 41 ↗ | (On Diff #328730) | See my comment below re compiler bug exposition. |
| libcxx/test/std/concepts/object/copyable.compile.pass.cpp | ||
|---|---|---|
| 63 | D74291: You mean Casey's comment "the MSVC STL tests for type traits tend to be the tests for compiler intrinsics used to implement those traits"? I would like some libc++ maintainer's opinion on that topic. I have only my hopes and preconceptions to go on ;) but I hope that (A) Clang has its own set of tests for its compiler intrinsics; (B) libc++ has very thorough tests for <type_traits>, such that we don't need to repeat that same laundry-list in the <concepts> tests; and... (C), the problem I have with "testing for defects in the compiler" is that if that's the purpose, then I'd like to know what is your plan for when such a defect is exposed! Let's suppose for the sake of argument that GCC trunk suddenly starts miscompiling struct S { const int i; } and reporting that it is_assignable. GCC's own tests don't catch the bug, and it is released into the wild. libc++'s tests for std::assignable go red. How do we make them green again? If the answer is "add a workaround in the <concepts> header," then sure, it makes sense for the <concepts> tests to be testing this case. But I'm having a hard time imagining what such a workaround would even look like. <concepts> is super high-level. It feels like testing basic arithmetic in the libc++ tests for <vector>: sure a compiler could miscompile 2+2=4 and that would break our vector implementation, but I'm not sure what the "maintainer of vector" would actually do about that. Obviously this is all quite fuzzy. Also, it might be that our <type_traits> tests are abysmal, and therefore we should graciously accept your improved <concepts> tests (because you would have no interest in improving the <type_traits> tests, and tests here are better than tests nowhere). ...actually I just glanced at So. I will try to keep in mind that these new tests are basically (C++20-mode-only) improvements on our currently abysmal <type_traits> tests. I'll grant the relevance of the has_volatile_member etc. struct types on that basis. However, even so, I would still cut these 25 lines down to 1, those 4 cv-qualified-pointee lines down to 1, etc. I still claim that not even a buggy compiler would ever believe that e.g. int (S::*)() volatile&& is copyable but int (S::*)() const volatile& noexcept is not. | |
| libcxx/test/std/concepts/object/copyable.h | ||
|---|---|---|
| 34 ↗ | (On Diff #328730) | I believe the library often uses const T when it means const T&&. (For example, forward<T> means forward<T&&>; declval<T> means declval<T&&>; etc.) Assuming the same holds true here, you could do a struct with an overload set like struct deleted_assignment_from_const_rvalue {
deleted_assignment_from_const_rvalue& operator=(const deleted_assignment_from_const_rvalue&);
deleted_assignment_from_const_rvalue& operator=(deleted_assignment_from_const_rvalue&&);
deleted_assignment_from_const_rvalue& operator=(const deleted_assignment_from_const_rvalue&&) = delete;
};
static_assert(!std::assignable<deleted_assignment_from_const_rvalue&, const deleted_assignment_from_const_rvalue>);
static_assert(!std::copyable<deleted_assignment_from_const_rvalue>); |
| libcxx/test/std/concepts/object/copyable.compile.pass.cpp | ||
|---|---|---|
| 63 |
Yeah. /summons @CaseyCarter, whom we're quoting.
@ldionne and @EricWF have had this to say regarding my comprehensive tests. Granted, I might be taking their feedback out of context, and am almost certainly projecting my own thoughts here, so having one or both of them weigh in would be good.
—Louis, https://reviews.llvm.org/D77961
—Eric, https://reviews.llvm.org/D81823#inline-752457
🤷 test suites can be incomplete, and this exposes them.
I think Louis addresses that in D96742. The comment was in regard to library issues within libc++, but I don't see why it can't be generalised to compiler defects as well.
GCC issue 99374 is already fixed in trunk and based on the new title of the report, it looks like it'll be fixed in versions 8-10 too.
Don't go and look at libc++'s comparison operator tests for containers D:
Not right now. I fully intend to audit libc++'s test coverage at some point in the future, once the C++20 (and possibly C++23 if it takes that long) ranges stuff is in. (This isn't a promise, just an indication of what I'd like to do one day in the near future.)
Ack.
I can see what you're saying (a pointer-to-member function is a pointer-to-member function is a pointer-to-member function), but GCC issue 99374 has me spooked. Since libc++ approval requires two co-approver to LGTM, I think it would be worth waiting for a second co-approver to offer their 2c. If two approvers | |
| libcxx/test/std/concepts/object/copyable.compile.pass.cpp | ||
|---|---|---|
| 112 | Fair enough, let's keep .compile.pass then. | |
| libcxx/test/std/concepts/object/copyable.h | ||
| 1 ↗ | (On Diff #328730) | Yeah, there might also be some places that it would help to have, say, a no_copy_constructor type. Some of this stuff can be used more generally whereas some of this looks like it's more single-use. I'd be in favor of moving the stuff we want to use more generally to the support directory. Or maybe support/type-classification or support/concepts. |
Comment Actions
LGTM. Since they @zoecarver and @Quuxplusone have some comments I leave the final approval to them.
| libcxx/test/std/concepts/object/copyable.compile.pass.cpp | ||
|---|---|---|
| 63 | I'm also not too fond of the verbose tests and also feel they should do a short sanity check whether the underlying concepts and type_traits are properly used. They shouldn't test whether these underlying concepts and type_traits are implemented correctly. However other reviewers have requested verbose tests in the past and other accepted concepts have these verbose sets. So based on that I'd like to keep the verbose tests. | |
| 112 | .verify.cpp can be used to test for expected compiler warnings/errors. | |
Comment Actions
Accepted, in that the code is obviously correct and we're unlikely ever to agree on the "right" tests. 😛
| libcxx/test/std/concepts/object/copyable.h | ||
|---|---|---|
| 53 ↗ | (On Diff #328825) | This naming convention implies more structure than you really have here. I think struct X might be a better name for this particular grab-bag of special member signatures. struct volatile_copy_assignment {
volatile_copy_assignment(volatile_copy_assignment const volatile&);
volatile_copy_assignment(volatile_copy_assignment const volatile&&);
volatile_copy_assignment volatile& operator=(volatile_copy_assignment const volatile&) volatile;
volatile_copy_assignment volatile& operator=(volatile_copy_assignment const volatile&&) volatile;
};(It's trickier than it sounds at first glance, because even though an X&& argument binds to a const X& parameter, yet a volatile X&& argument does not bind to a const volatile X& parameter.) |
| 1 ↗ | (On Diff #328730) |
(1) Yes, and, we actually do have a class MoveOnly (which behaves basically like int but is move-only) which I've been using heavily in the STL-algorithm tests. |
| libcxx/test/std/concepts/object/copyable.compile.pass.cpp | ||
|---|---|---|
| 63 | Something I plan to do post-finishing P0898, pre-moving on to P0896, are subsumption tests. This will let me delete most of the repeated tests in libcxx/test/std/concepts, albeit in a later revision. tl;dr I'm committed to keeping tests in active revisions as-is, but will work on a way to reduce repetitiveness and improve coverage. | |
| libcxx/test/std/concepts/object/copyable.h | ||
|---|---|---|
| 53 ↗ | (On Diff #328825) | Isn't that what cv_copy_assignment does below? |
| libcxx/test/std/concepts/object/copyable.h | ||
|---|---|---|
| 53 ↗ | (On Diff #328825) | No, cv_copy_assignment looks like it's trying to achieve std::copyable<const volatile cv_copy_assignment>. It does not achieve std::copyable<volatile cv_copy_assignment>, because the return type of operator= is wrong for that. |
| libcxx/test/std/concepts/object/copyable.compile.pass.cpp | ||
|---|---|---|
| 99 | I think you should change volatile_copy_assignment as indicated above, so that you can remove the ! character on line 97. Then lines 96,97,98 will be parallel to each other, which will be good. | |
Comment Actions
LGTM. I don't want to squash the conversation here, but I think we've discussed it enough. The actual changes here look solid, and I think they should be allowed to proceed.
I see nothing testworthy about these four pointer types. The cv-qualification of the pointed-to type is obviously irrelevant to the copyability of the pointer itself.
However, it might be interesting to test some top-level-cv-qualified class types, i.e. const S and volatile S, especially w.r.t. when they have (or don't have) const-qualified assignment operators.