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

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

Quuxplusone marked 3 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/__concepts/copyable.h:10
+#ifndef _LIBCPP___CONCEPTS_COPYABLE
+#define _LIBCPP___CONCEPTS_COPYABLE
+
----------------
Mordante wrote:
> To be more in line with `constructible.h` how do you feel to name this file `assignable.h`?
> Mainly since these two headers contain multiple concepts.
Hmm, I see what you mean. The only library consumers of `concept assignable_from` are `concept movable` and `concept copyable` (and `concept swappable`, which already depends on all of the above; and `concept indirectly_movable_storable`, which already depends on `movable`). So perhaps these three should be moved into the same file.

However, @cjdb's objection that the name of the file isn't obvious would still apply: `assignable_from`, `movable`, and `copyable` have very similar //semantics// but very different //spellings//. So I'm inclined to just split up `movable.h` from `copyable.h` as the path of least resistance.

As with the `relation` concepts, I specifically don't want to put `concept copyable` and `concept regular` in the same file because I don't want `copyable` to depend on the comparison traits.


================
Comment at: libcxx/include/__concepts/equality_comparable.h:10
+#ifndef _LIBCPP___CONCEPTS_EQUALITY_COMPARABLE
+#define _LIBCPP___CONCEPTS_EQUALITY_COMPARABLE
+
----------------
cjdb wrote:
> 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
Data point: This file defines both `concept equality_comparable` and `concept equality_comparable_with` (just as `swappable.h` defines both `swappable` and `swappable_with`). I suspect @Mordante may have missed that. `concept equality_comparable` is just 2 lines long (and surprisingly is //not// just `equality_comparable_with<T, T>`).


================
Comment at: libcxx/include/__concepts/invocable.h:34-35
+
+template<class _Fn, class... _Args>
+concept regular_invocable = invocable<_Fn, _Args...>;
+
----------------
cjdb wrote:
> 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.
I prefer not to split `invocable` from `regular_invocable`, because they are (as far as libc++ is concerned) equivalent.
However, it's good to split this concept from `predicate` and `relation` (and the other concepts equivalent to `relation`) because //this// file doesn't depend on `__boolean_testable` but //those// concepts do.


================
Comment at: libcxx/include/__concepts/relation.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
cjdb wrote:
> 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`
I'd prefer not to split further; but I won't object to splitting `predicate` from `relation` just for the heck of it. It's kind of intuitive in that `concept predicate` has a spelling not involving the word `relation`, whereas all the other equivalent concepts //do// involve the word `relation`. So I'll split into `predicate.h` and `relation.h`, unless someone stops me first.


================
Comment at: libcxx/include/__concepts/same_as.h:26
+template<class _Tp, class _Up>
+concept __same_as_impl = _VSTD::_IsSame<_Tp, _Up>::value;
+
----------------
Mordante wrote:
> Can you do a separate commit to remove the `_VSTD`?
Sure.


================
Comment at: libcxx/include/concepts:1
-// -*- C++ -*-
-//===-------------------------- concepts ----------------------------------===//
+//===----------------------------------------------------------------------===//
 //
----------------
cjdb wrote:
> Mordante wrote:
> > Please restore `// -*- C++ -*-`
> Why?
Will restore, for consistency with `<vector>` etc.
I had not noticed that every single header (in the top-level `include/` directory anyway) does this.
Other than the consistency argument, I agree with @cjdb's "Why?": perhaps we should float a PR to kill this line from //all// libc++ headers, but of course separately from D107584.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107584