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

Fri Aug 6 09:22:42 PDT 2021

cjdb added inline comments.

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

================
Comment at: libcxx/include/concepts:133-136
-#include <__functional/invoke.h>
-#include <__functional_base>
-#include <type_traits>
-#include <utility>
----------------
Quuxplusone wrote:
> @ldionne: Today, this works:
> ```
> #include <concepts>
> std::true_type f();
> // or std::move, std::forward, etc.
> ```
> What I propose here, will break Modules builds for people who do this.
> Notably, our own tests do it in these files:
> ```
> 	modified:   test/std/concepts/concepts.callable/concept.predicate/predicate.compile.pass.cpp
> 	modified:   test/std/concepts/concepts.callable/concept.predicate/predicate.pass.cpp
> 	modified:   test/std/concepts/concepts.lang/concept.common/common_with.compile.pass.cpp
> 	modified:   test/std/concepts/concepts.lang/concept.commonref/common_reference.compile.pass.cpp
> ```
> Alternatives to choose among:
> (1) Since `<concepts>` is super new (C++20) and we have a chance to "do it right," personally I'm inclined to take this breakage and just fix our four offending tests.
> (2) Or, fix the tests but leave the three top-level `#include`s here. (`<__functional/invoke.h>` is already a private module, thus safe to remove. `<__functional_base>` is not in the modulemap at all.)
> (3) Or, leave the three top-level `#include`s and deliberately //do not// fix the tests, and/or add some new regression tests in `test/libcxx/` verifying that libc++'s `<concepts>` continues to provide `true_type`, `basic_common_reference`, `move`, and so on.
I'm inclined to agree with (1). A significantly smaller user-base than our older headers should mean a lot less pain (if any).

If we go down route (3), I'd ask that we fix the existing tests and add a (temporary) regression test.

================
Comment at: libcxx/include/concepts:1
-// -*- C++ -*-
-//===-------------------------- concepts ----------------------------------===//
+//===----------------------------------------------------------------------===//
 //
----------------
Mordante wrote:
> Quuxplusone wrote:
> > 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.
> This line is used in Emacs and Vim to enable the proper syntax highlighting. When removed these editors show no syntax highlighting on these files.
TIL. Let's keep them in all extensionless headers then.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107584