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

Arthur O'Dwyer via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Sun Mar 7 17:03:56 PST 2021

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;
(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; };

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list