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

[Louis Dionne via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/include/algorithm:1263-1268
+#ifndef _LIBCPP_REMOVE_TRANSITIVE_INCLUDES
+#  include <chrono>
+#  include <functional>
+#  include <iterator>
+#  include <utility>
+#endif
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128661