[libcxx-commits] [PATCH] D97911: [libcxx] adds concept std::semiregular

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

Quuxplusone accepted this revision.
Quuxplusone added a comment.

Code looks good, tests look too long — you know the drill.



================
Comment at: libcxx/test/std/concepts/object/copyable.h:39
 struct const_copy_assignment {
+  const_copy_assignment();
+
----------------
I'm confused by this change. Was `const_copy_assignment` already used in the tests for `std::copyable`? Were those tests intended to check that it was `std::copyable` despite being not-default-initializable? By making it default-initializable (and thus `std::semiregular`), are you nerfing the existing `std::copyable` tests?


================
Comment at: libcxx/test/std/concepts/object/semiregular.compile.pass.cpp:12
+
+// template<class From>
+// concept semiregular = see below;
----------------
s/From/T/
(and this probably affects other patches as well)


================
Comment at: libcxx/test/std/concepts/object/semiregular.h:18
+  std::in_place_t x;
+};
+
----------------
We don't need to drag in a dependency on `optional` here. Also, it's not obvious to me that `nullopt_t` is inheritable-from; certainly users //should not// do that. Therefore I strongly recommend replacing this with the following:

```
struct no_default_ctor { no_default_ctor(int); };
struct derived_from_non_default_initializable : no_default_ctor {};
struct has_non_default_initializable { no_default_ctor x; };

// and/or perhaps also test one with a deleted default ctor? but that'll double the size of the tests again
struct deleted_default_ctor { deleted_default_ctor() = delete; };
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97911