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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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 "")
----------------
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).


================
Comment at: libcxx/include/algorithm:1263-1268
+#ifndef _LIBCPP_REMOVE_TRANSITIVE_INCLUDES
+#  include <chrono>
+#  include <functional>
+#  include <iterator>
+#  include <utility>
+#endif
----------------
ldionne wrote:
> philnik wrote:
> > I would put the includes below the standard-mandated ones, but I guess it doesn't make a big difference, since we plan to remove them anyways.
> I'll keep as-is, because changing this would be a sizeable amount of work for little benefit, especially since I have other follow-up patches locally that would need a ton of rebasing. I'll be happy to do it if there's a technical reason to, but I can't think of one right now. LMK if you do.
It's just preference - the stuff we don't want to include shouldn't provide dependencies for other headers. This is of course a moot point with a modules build. So yeah, I don't care that much (especially if it would cause a lot of work for you).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128661