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

Sun Aug 15 16:20:28 PDT 2021

cjdb added inline comments.

================
Comment at: libcxx/include/__concepts/boolean_testable.h:30
+concept __boolean_testable = __boolean_testable_impl<_Tp> && requires(_Tp&& __t) {
+  { !static_cast<_Tp&&>(__t) } -> __boolean_testable_impl;
+};
----------------
We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made.

================
Comment at: libcxx/include/__concepts/invocable.h:34-35
+
+template<class _Fn, class... _Args>
+concept regular_invocable = invocable<_Fn, _Args...>;
+
----------------
cjdb wrote:
> 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).
Ping.

================
Comment at: libcxx/include/__concepts/swappable.h:65
+    template<class _Tp, class _Up>
+      requires __unqualified_swappable_with<_Tp, _Up>
+    constexpr void operator()(_Tp&& __t, _Up&& __u) const
----------------
We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made.

================
Comment at: libcxx/include/__concepts/swappable.h:67
+    constexpr void operator()(_Tp&& __t, _Up&& __u) const
+      noexcept(noexcept(swap(_VSTD::forward<_Tp>(__t), _VSTD::forward<_Up>(__u))))
+    {
----------------
We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made.

================
Comment at: libcxx/include/__concepts/swappable.h:74
+    template<class _Tp, class _Up, size_t _Size>
+      requires __swappable_arrays<_Tp, _Up, _Size>
+    constexpr void operator()(_Tp(& __t)[_Size], _Up(& __u)[_Size]) const
----------------
We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made.

================
Comment at: libcxx/include/__concepts/swappable.h:76
+    constexpr void operator()(_Tp(& __t)[_Size], _Up(& __u)[_Size]) const
+      noexcept(noexcept((*this)(*__t, *__u)))
+    {
----------------
We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made.

================
Comment at: libcxx/include/__concepts/swappable.h:87
+    constexpr void operator()(_Tp& __x, _Tp& __y) const
+      noexcept(is_nothrow_move_constructible_v<_Tp> && is_nothrow_move_assignable_v<_Tp>)
+    {
----------------
We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107584