[libcxx-commits] [PATCH] D97443: [libcxx] adds concept std::copyable

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked 3 inline comments as done.
cjdb added inline comments.


================
Comment at: libcxx/test/std/concepts/object/copyable.compile.pass.cpp:33
+static_assert(std::copyable<int volatile*>);
+static_assert(std::copyable<int volatile const*>);
+static_assert(std::copyable<int (*)()>);
----------------
Quuxplusone wrote:
> 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.
> 
> 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.


================
Comment at: libcxx/test/std/concepts/object/copyable.compile.pass.cpp:62
+static_assert(std::copyable<int (S::*)() const volatile&&>);
+static_assert(std::copyable<int (S::*)() const volatile && noexcept>);
+
----------------
Quuxplusone wrote:
> 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.
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.


================
Comment at: libcxx/test/std/concepts/object/copyable.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > 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). 
> 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.
> 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).


================
Comment at: libcxx/test/std/concepts/object/copyable.h:34
+  no_const_value_assignment&
+  operator=(no_const_value_assignment const) = delete;
+};
----------------
Quuxplusone wrote:
> 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.)
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.


================
Comment at: libcxx/test/std/concepts/object/copyable.h:41
+  std::unique_ptr<int> x;
+};
+#endif // TEST_STD_CONCEPTS_OBJECT_COPYABLE_H
----------------
Quuxplusone wrote:
> I see nothing testworthy about `derived_from_noncopyable` or `has_noncopyable`.
See my comment below re compiler bug exposition.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97443/new/

https://reviews.llvm.org/D97443