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

Wed Mar 10 09:41:00 PST 2021

Mordante added a comment.

LGTM. Since they @zoecarver and @Quuxplusone have some comments I leave the final approval to them.

================
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:
> > 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.
> > 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 
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.

================
Comment at: libcxx/test/std/concepts/object/copyable.compile.pass.cpp:111
+
+int main(int, char**) { return 0; }
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > 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`. :)
> Fair enough, let's keep `.compile.pass` then. 
`.verify.cpp` can be used to test for expected compiler warnings/errors.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97443