[libcxx-commits] [PATCH] D128236: [libc++] Add a test to pin down the set of transitive public includes

Wed Jun 22 09:30:14 PDT 2022

ldionne added inline comments.

================
Comment at: libcxx/test/libcxx/transitive_includes.sh.cpp:15
+// forever, however we do try to group removals for a couple of releases
+// to avoid breaking users at every release.
+
----------------
Mordante wrote:
> Would it be an idea to add a "schedule" here too? Basically to avoid people working on a cleanup patch and have it rejected just because it's not "the right time". For example only cleanup when the next version number is a multiple of 3. (3 just because 15 is a multiple of 3.)
> (But maybe we should discuss the details on Discord.)
When I first read this comment, I was thinking "this is a great idea". Then I gave it some more thought and I am not so sure. Indeed, I don't think we should be held to a specific (and arbitrary) schedule for breaking transitive header dependencies: instead we should do it in a reasoned fashion when it makes the most sense. For example, we are going to ship large parts of Ranges in LLVM 15, so making the transition to LLVM 15 as seamless as possible is desirable for users to get the benefits of Ranges ASAP. So much so that my plan was actually to go back and re-add some headers we've removed since branching LLVM 14 to avoid breaking folks in this release.

After all, I don't think we would reject any cleanup patch because it's "not the right time". We would simply add some transitive includes for backwards compatibility with a `TODO` above them, so the cost here is really small.

TLDR: Let's be very intentional about removing these transitive dependencies, and we can even give a heads up in advance for people to get prepared. The amount of breakage that this can cause (and the barrier it adds to adoption of a new libc++) is surprisingly high.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128236