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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

As usual, the code in `<concepts>` seems obviously correct, and the tests seem too verbose.



================
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 (*)()>);
----------------
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.



================
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>);
+
----------------
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.


================
Comment at: libcxx/test/std/concepts/object/copyable.compile.pass.cpp:111
+
+int main(int, char**) { return 0; }
----------------
zoecarver wrote:
> zoecarver wrote:
> > I think this test can/should be a `.verify.cpp` test because there's not actually any runtime logic. 
> I just saw this is marked as `compile.pass`, I suppose that does the same thing.
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`. :)


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


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


================
Comment at: libcxx/test/std/concepts/object/copyable.h:41
+  std::unique_ptr<int> x;
+};
+#endif // TEST_STD_CONCEPTS_OBJECT_COPYABLE_H
----------------
I see nothing testworthy about `derived_from_noncopyable` or `has_noncopyable`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97443