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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/test/std/concepts/object/copyable.compile.pass.cpp:111
+
+int main(int, char**) { return 0; }
----------------
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. 


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97443