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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb accepted this revision.
cjdb added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/__concepts/invocable.h:34-35
+
+template<class _Fn, class... _Args>
+concept regular_invocable = invocable<_Fn, _Args...>;
+
----------------
ldionne wrote:
> cjdb wrote:
> > Quuxplusone wrote:
> > > cjdb wrote:
> > > > Quuxplusone wrote:
> > > > > cjdb wrote:
> > > > > > Quuxplusone wrote:
> > > > > > > cjdb wrote:
> > > > > > > > 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.
> > > > > > > `__boolean_testable` is an ancestor of both `equality_comparable` and `predicate`.
> > > > > > Yes, but `__boolean_testable` has its own header. I'm trying to work out why you're opposed to combining the all the callables into a single header, and you pointed at `__boolean_testable`.
> > > > > I must not understand what you're asking, because it //seems// like this is really basic.
> > > > > My initial PR had:
> > > > > `invocable` and `regular_invocable` go in `invocable.h`, which does not include `boolean_testable.h`.
> > > > > `predicate` and `relation` and `equivalence_relation` and so on go in `relation.h`, which does include `boolean_testable.h`.
> > > > > 
> > > > > My current PR splits `predicate` into `predicate.h`, purely on the basis of name-spelling, because you asked for it.
> > > > > 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.
> > > > 
> > > > You've said that `predicate`, `relation`, etc., depending on `__boolean_testable` is motivation for not consolidating into a single `callable.h`. You haven't outlined why it's motivation.
> > > > 
> > > > It's also occurred to me overnight that @ldionne may want a consolidated header, as he did for `<functional>`'s function objects. Again, I'm ambivalent on that, but it's something Louis might have a stronger opinion on.
> > > @ldionne, could you put this subthread to rest, please?
> > Specifically, this one.
> I am neutral on which one of the two following solutions are taken:
> 
> 1. `callable.h` that contains all the "roughly callable" concepts, like `invocable`, `regular_invocable`, `predicate`, etc.
> 2. the status quo, where `invocable.h` defines both `invocable` and `regular_invocable`
> 
> The one thing I'd much rather avoid is create a new header for just `regular_invocable`, since it's so trivial.
> 
> All those preferences are very, very tiny IMO. I'm basically neutral - I think it simply doesn't matter much because all those solutions make sense to me. I think we should just pick something and move forward with this patch to unblock more important work.
Looks like there's an answer, but I'd still like an answer for the dependency concern on the record (you can answer that after merging though).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107584