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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/concepts:133-136
-#include <__functional/invoke.h>
-#include <__functional_base>
-#include <type_traits>
-#include <utility>
----------------
cjdb wrote:
> 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.
I would say definitely go with (1) too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107584