[libcxx-commits] [PATCH] D107584: [libc++][modularisation] Split up <concepts> into granular headers.

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb requested changes to this revision.
cjdb added a comment.
This revision now requires changes to proceed.

Looking good, a few things to work out though.



================
Comment at: libcxx/include/__concepts/copyable.h:28-33
+template<class _Tp>
+concept movable =
+  is_object_v<_Tp> &&
+  move_constructible<_Tp> &&
+  assignable_from<_Tp&, _Tp> &&
+  swappable<_Tp>;
----------------
It's not immediately obvious that `movable` is in `copyable.h`. Please either:

* split up further
* consolidate with `semiregular` and `regular`, and call the file `object.h`

I'm okay with either approach.


================
Comment at: libcxx/include/__concepts/equality_comparable.h:10
+#ifndef _LIBCPP___CONCEPTS_EQUALITY_COMPARABLE
+#define _LIBCPP___CONCEPTS_EQUALITY_COMPARABLE
+
----------------
Mordante wrote:
> Shouldn't this file be named `equality_comparable_with.h`?
I think `equality_comaprable` is a better name for two reasons:

1. it's shorter to type and is a catch all for search tools
2. it captures the notion of equality comparability


================
Comment at: libcxx/include/__concepts/invocable.h:34-35
+
+template<class _Fn, class... _Args>
+concept regular_invocable = invocable<_Fn, _Args...>;
+
----------------
It's not immediately clear that `regular_invocable` lives in `invocable.h`. Please either:

* split the file up further
* consolidate with the other callable concepts and call the header `callable.h`

I'm indifferent to the chosen approach, but please be consistent in the approach you choose.


================
Comment at: libcxx/include/__concepts/relation.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
If you choose to split up `invocable.h`, please also split up this file. The two splices I'm okay with:

* one concept per file
* `predicate.h` and `relation.h`


================
Comment at: libcxx/include/concepts:1
-// -*- C++ -*-
-//===-------------------------- concepts ----------------------------------===//
+//===----------------------------------------------------------------------===//
 //
----------------
Mordante wrote:
> Please restore `// -*- C++ -*-`
Why?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107584