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

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

Quuxplusone added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97443