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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/__concepts/equality_comparable.h:10
+#ifndef _LIBCPP___CONCEPTS_EQUALITY_COMPARABLE
+#define _LIBCPP___CONCEPTS_EQUALITY_COMPARABLE
+
----------------
Quuxplusone wrote:
> 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>`).
> (and surprisingly is not just `equality_comparable_with<T, T>`).

The reason for this is because it optimises out the expensive `common_reference_with` check when we know it's true (otherwise we'd just have a binary `equality_comparable`, where `U = T`).


================
Comment at: libcxx/include/__concepts/invocable.h:34-35
+
+template<class _Fn, class... _Args>
+concept regular_invocable = invocable<_Fn, _Args...>;
+
----------------
Quuxplusone wrote:
> 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.
:shrug: I guess the same argument that I made for `equality_comparable.h` vs `equality_comparable_with.h` could be made here re not splitting further. Though, I'm not sure why you're opposed to `__boolean_testable` being in this header. Ditto for `object.h`, but we should keep the conversation in one place.


================
Comment at: libcxx/include/__concepts/relation.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
Quuxplusone wrote:
> 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.
Yeah, that's what I meant by `predicate.h` and `relation.h`. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107584