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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

Accepted, in that the code is obviously correct and we're unlikely ever to agree on the "right" tests. 😛



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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97443