[libcxx-commits] [PATCH] D128661: [libc++] Re-add transitive includes that had been removed since LLVM 14

[Louis Dionne via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/cmake/caches/Generic-no-transitive-includes.cmake:1
+set(LIBCXX_TEST_PARAMS "enable_transitive_includes=False" CACHE STRING "")
+set(LIBCXXABI_TEST_PARAMS "${LIBCXX_TEST_PARAMS}" CACHE STRING "")
----------------
philnik wrote:
> ldionne wrote:
> > philnik wrote:
> > > I think the right way would be to remove the transitive includes by default in the CI and disable them in one runner.
> > IMO the CI should mirror the state of the library as we're going to release/ship it. It makes it more likely that we'll catch errors that we would otherwise ship. Since we want to ship with the transitive includes enabled by default, all CI jobs should have them by default.
> > 
> > The added CI job only exists to ensure that the code doesn't bit rot and that we can remove the transitive includes in the future without additional work.
> I would normally agree, but in this case I don't see how it could break anything. We have tests to ensure that we can include everything in a single TU, a Modules build and more to ensure all the includes works together properly. I would see it the way same as `_LIBCPP_HAS_NO_INCOMPLETE_RANGES` - we work on it and check it in the CI, but we don't ship it yet (plus we have a CI job for the "disabled" configuration).
Yeah, I can see the parallel with `_LIBCPP_HAS_NO_INCOMPLETE_RANGES`, however I do think that this option leads to complexity and brittleness (for example we have to turn it `ON` on our release branches while it's `OFF` on `main`). Concretely, I think having a CMake cache for LLVM releases would solve most of my concerns about this, except for the need to have a CMake-level configuration option for this knob: If we change it to `_LIBCPP_ENABLE_BACKWARDS_COMPAT_TRANSITIVE_INCLUDES` and make it undefined by default, we'll need a setting in `__config_site` to define it by default for LLVM releases and for most vendors. I'd like to avoid this additional complexity.

Also, to further make a distinction with `_LIBCPP_HAS_NO_INCOMPLETE_RANGES`, once we move to `-funstable`, `_LIBCPP_HAS_NO_INCOMPLETE_RANGES` will be disabled by default (unlike right now), so it will be consistent with the approach proposed here. I'll land this patch now because I want to see how it affects downstreams immediately, but I don't consider this discussion to be closed -- I'm open to discussion on the default ON/OFF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128661