[libcxx] adds concept std::copyable

[cjdb]

Summary

Implements parts of:

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

Depends on D97359

[cjdb]

uglifies names

[cjdb]

rebases to activate CI

[cjdb]

rebases and fixes things up to keep in line with prior commits

[cjdb]

rebases to pass CI

[cjdb]

deletes file that was copied instead of being moved

[zoecarver]

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

2

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).

[• Quuxplusone]

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

2

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.

35

The const keyword on a by-value function parameter is meaningless; please remove it. (And then rename to no_assignment.)
I don't intuitively understand what overload set is expected here. Do you expect there to be a defaulted operator=(const T&) and/or operator=(T&&)? (I think there won't be.)

42

I see nothing testworthy about derived_from_noncopyable or has_noncopyable.

[cjdb]

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.

See below.

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.

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.
Without these sorts of seemingly random tests, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99374 wouldn't have been discovered.

libcxx/test/std/concepts/object/copyable.h

2

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).

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.

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.

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).

35

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.

42

See my comment below re compiler bug exposition.

[• Quuxplusone]

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
libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_assignable.pass.cpp
and guess what, it is pretty bare-bones! :(

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.

[• Quuxplusone]

libcxx/test/std/concepts/object/copyable.h

35

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>);

[cjdb]

libcxx/test/std/concepts/object/copyable.compile.pass.cpp

63

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"?

Yeah. /summons @CaseyCarter, whom we're quoting.

I would like some libc++ maintainer's opinion on that topic. I have only my hopes and preconceptions to go on ;)

@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.

I think we could do (1). In the meantime, let's try to make sure we have good test coverage by ourselves (as you did here).

—Louis, https://reviews.llvm.org/D77961

I like tests. Let's keep them.

—Eric, https://reviews.llvm.org/D81823#inline-752457

---

(A) Clang has its own set of tests for its compiler intrinsics

🤷 test suites can be incomplete, and this exposes them.

---

(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?

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.

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.

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.

---

(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...

...actually I just glanced at
libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/is_assignable.pass.cpp
and guess what, it is pretty bare-bones! :(

Don't go and look at libc++'s comparison operator tests for containers D:

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).

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.)

I'll grant the relevance of the has_volatile_member etc. struct types on that basis.

Ack.

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.

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

[cjdb]

applies some of @Quuxplusone's feedback

[zoecarver]

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

2

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.

[Mordante]

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.

[• Quuxplusone]

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

2

there might also be some places that it would help to have, say, a no_copy_constructor type

(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.
(2) Unfortunately for Chris's purposes (stress-testing the compiler), there's a big difference between "no copy ctor" and "deleted copy ctor" and maybe even "copy ctor deleted by being defaulted" and so on and so forth. I think a systematic approach here would be (a) super time-consuming and (b) better moved into the Clang compiler test suite.

54

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.
Alternatively, it might arguably be important to test some type that is copyable but volatile-qualified. In that case, I think the minimal set of SMFs that makes the type actually copyable would be
https://godbolt.org/z/8affTh

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.)

[cjdb]

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.
I'd like some time to step back and think about how this would be cleanly implemented, hence why it'll come after what's already in flight (I'll pilot it in totally_ordered[_with]).

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.

[cjdb]

libcxx/test/std/concepts/object/copyable.h

54

Isn't that what cv_copy_assignment does below?

[• Quuxplusone]

libcxx/test/std/concepts/object/copyable.h

54

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.

[• Quuxplusone]

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.
Lines 96,97,98 have nothing to do with the comment // Not assignable on line 93. I recommend moving lines 96,97,98 up to line 74-ish.

[cjdb]

applies @zoecarver's suggestion to move headers

[cjdb]

responds to https://reviews.llvm.org/D97911#inline-920950

[EricWF]

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.

[zoecarver]

Sorry for the delay. This LGTM. Thanks for putting the headers in support!

libcxx/include/concepts

 // [concepts.object]
 template<class _Tp>
 concept movable =
   is_object_v<_Tp> &&
   move_constructible<_Tp> &&
   assignable_from<_Tp&, _Tp> &&
   swappable<_Tp>;
 
+template<class _Tp>
+concept copyable =
+  copy_constructible<_Tp> &&
+  movable<_Tp> &&
+  assignable_from<_Tp&, _Tp&> &&
+  assignable_from<_Tp&, const _Tp&> &&
+  assignable_from<_Tp&, const _Tp>;
+
 // [concept.invocable]
 template<class _Fn, class... _Args>
 concept invocable = requires(_Fn&& __fn, _Args&&... __args) {
   _VSTD::invoke(_VSTD::forward<_Fn>(__fn), _VSTD::forward<_Args>(__args)...); // not required to be equality preserving
 };
 
 // [concept.regular.invocable]
 template<class _Fn, class... _Args>

libcxx/test/std/concepts/object/copyable.h

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#ifndef TEST_STD_CONCEPTS_OBJECT_COPYABLE_H
+#define TEST_STD_CONCEPTS_OBJECT_COPYABLE_H
+
+#include <memory>
+
+#include "movable.h"
+
+struct no_copy_constructor {
+  no_copy_constructor(no_copy_constructor const&) = delete;
+  no_copy_constructor(no_copy_constructor&&) = default;
+};
+
+struct no_copy_assignment {
+  no_copy_assignment& operator=(no_copy_assignment const&) = delete;
+  no_copy_assignment& operator=(no_copy_assignment&&) = default;
+};
+
+struct no_copy_assignment_mutable {
+  no_copy_assignment_mutable&
+  operator=(no_copy_assignment_mutable const&) = default;
+  no_copy_assignment_mutable& operator=(no_copy_assignment_mutable&) = delete;
+  no_copy_assignment_mutable& operator=(no_copy_assignment_mutable&&) = default;
+};
+
+struct no_const_value_assignment {
+  no_const_value_assignment&
+  operator=(no_const_value_assignment const) = delete;
+};
+
+struct derived_from_noncopyable : std::unique_ptr<int> {};
+
+struct has_noncopyable {
+  std::unique_ptr<int> x;
+};
+#endif // TEST_STD_CONCEPTS_OBJECT_COPYABLE_H

libcxx/test/std/concepts/object/copyable.compile.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+
+// template<class From>
+// concept copyable = see below;
+
+#include <concepts>
+
+#include <deque>
+#include <forward_list>
+#include <list>
+#include <map>
+#include <memory>
+#include <optional>
+#include <unordered_map>
+#include <vector>
+
+#include "copyable.h"
+
+static_assert(std::copyable<int>);
+static_assert(std::copyable<int volatile>);
+static_assert(std::copyable<int*>);
+static_assert(std::copyable<int const*>);
+static_assert(std::copyable<int volatile*>);
+static_assert(std::copyable<int volatile const*>);
+static_assert(std::copyable<int (*)()>);
+
+struct S {};
+static_assert(std::copyable<S>);
+static_assert(std::copyable<int S::*>);
+static_assert(std::copyable<int (S::*)()>);
+static_assert(std::copyable<int (S::*)() noexcept>);
+static_assert(std::copyable<int (S::*)() &>);
+static_assert(std::copyable<int (S::*)() & noexcept>);
+static_assert(std::copyable<int (S::*)() &&>);
+static_assert(std::copyable<int (S::*)() && noexcept>);
+static_assert(std::copyable<int (S::*)() const>);
+static_assert(std::copyable<int (S::*)() const noexcept>);
+static_assert(std::copyable<int (S::*)() const&>);
+static_assert(std::copyable<int (S::*)() const & noexcept>);
+static_assert(std::copyable<int (S::*)() const&&>);
+static_assert(std::copyable<int (S::*)() const && noexcept>);
+static_assert(std::copyable<int (S::*)() volatile>);
+static_assert(std::copyable<int (S::*)() volatile noexcept>);
+static_assert(std::copyable<int (S::*)() volatile&>);
+static_assert(std::copyable<int (S::*)() volatile & noexcept>);
+static_assert(std::copyable<int (S::*)() volatile&&>);
+static_assert(std::copyable<int (S::*)() volatile && noexcept>);
+static_assert(std::copyable<int (S::*)() const volatile>);
+static_assert(std::copyable<int (S::*)() const volatile noexcept>);
+static_assert(std::copyable<int (S::*)() const volatile&>);
+static_assert(std::copyable<int (S::*)() const volatile & noexcept>);
+static_assert(std::copyable<int (S::*)() const volatile&&>);
+static_assert(std::copyable<int (S::*)() const volatile && noexcept>);
+
+static_assert(std::copyable<std::vector<int> >);
+static_assert(std::copyable<std::deque<int> >);
+static_assert(std::copyable<std::forward_list<int> >);
+static_assert(std::copyable<std::list<int> >);
+static_assert(std::copyable<std::shared_ptr<std::unique_ptr<int> > >);
+static_assert(std::copyable<std::optional<std::vector<int> > >);
+static_assert(std::copyable<std::vector<int> >);
+static_assert(std::copyable<std::vector<std::unique_ptr<int> > >);
+
+static_assert(std::copyable<has_volatile_member>);
+static_assert(std::copyable<has_array_member>);
+
+// Not objects
+static_assert(!std::copyable<void>);
+static_assert(!std::copyable<int&>);
+static_assert(!std::copyable<int const&>);
+static_assert(!std::copyable<int volatile&>);
+static_assert(!std::copyable<int const volatile&>);
+static_assert(!std::copyable<int&&>);
+static_assert(!std::copyable<int const&&>);
+static_assert(!std::copyable<int volatile&&>);
+static_assert(!std::copyable<int const volatile&&>);
+static_assert(!std::copyable<int()>);
+static_assert(!std::copyable<int (&)()>);
+static_assert(!std::copyable<int[5]>);
+
+// Not copy constructible or copy assignable
+static_assert(!std::copyable<std::unique_ptr<int> >);
+
+// Not assignable
+static_assert(!std::copyable<int const>);
+static_assert(!std::copyable<int const volatile>);
+
+static_assert(!std::copyable<no_copy_constructor>);
+static_assert(!std::copyable<no_copy_assignment>);
+
+static_assert(std::is_copy_assignable_v<no_copy_assignment_mutable>);
+static_assert(!std::copyable<no_copy_assignment_mutable>);
+static_assert(!std::copyable<no_const_value_assignment>);
+static_assert(!std::copyable<derived_from_noncopyable>);
+static_assert(!std::copyable<has_noncopyable>);
+static_assert(!std::copyable<has_const_member>);
+static_assert(!std::copyable<has_cv_member>);
+static_assert(!std::copyable<has_lvalue_reference_member>);
+static_assert(!std::copyable<has_rvalue_reference_member>);
+static_assert(!std::copyable<has_function_ref_member>);
+
+int main(int, char**) { return 0; }