[libcxx-commits] [PATCH] D132284: [libc++] Reduces the number of transitive includes.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst:32-33
+that users will have to fix their missing includes in order to upgrade to a
+newer version of the Standard. Libc++ also reserves the right to remove
+transitive includes at any other time, however new language versions will be
+used as a convenient way to perform bulk removals of
----------------
philnik wrote:
> Mordante wrote:
> > philnik wrote:
> > > Mordante wrote:
> > > > philnik wrote:
> > > > > Would the plan still be to remove transitive includes at some point? If not the includes will probably end up in a huge mess.
> > > > I'm not sure. We surely need to remove them when it breaks us, like I did for `<chrono>`.
> > > > The big issue of removal is that may break the packagers who then need to fix it.
> > > > If it was only developers I would have less of an issue with the breakage.
> > > > 
> > > > We can still remove them without repercussions from C++23 and I want to look at some more TODO removals and use the granularized headers in more places. Some header include `concepts` or `type_traits` which can be minimized now.
> > > > 
> > > > I'm not sure whether we would get in a huge mess with the current header method.
> > > > Do you have any concrete concerns with this approach?
> > > > 
> > > > 
> > > > 
> > > I'm fine with it if they're removed in a release where we don't get that much done that we can ship stable. LLVM 16 will definitely contain ranges and probably format, `std::pmr` (PRs coming soon), maybe the PSTL (@ldionne friendly reminder to work on D103198) and more. So that's most likely not a good option, but I don't think that there will be huge additions in LLVM 17. LLVM 15 would actually have been a really good release in hind-sight. The main reason we didn't do it then was because of ranges, but that didn't actually happen.
> > > 
> > > TL;DR I don't think it would be a huge problem if people don't have a really good reason to update (not that you shouldn't keep your tools up-to-date).
> > > 
> > > My main concern is that we will have to move stuff around to implement the rest of C++17 and C++20. Having
> > > ```
> > > #if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 14
> > > ...
> > > #endif
> > > 
> > > #if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 17
> > > ...
> > > #endif
> > > 
> > > 
> > > #if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
> > > ...
> > > #endif
> > > ```
> > > seems to me like a bad idea. Adding a few includes also isn't that huge of a deal. I'm not saying we should do it right now, but it might be a good idea to say: We'll remove all transitive includes in LLVM 17 (maybe?), so, vendors, please enable `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` for your internal testing and file bugs against any libraries/programs that break. That would give vendors lots of time to resolve any issues.
> > First of all I hope we can clean up our transitive includes before LLVM 16. Most of our larger headers have been granularized, we need to use them were appropriate.
> > 
> > I initially also considered to remove them fast, but we're not sure how much of effort that will be for vendors. If they have to file bugs against hundreds of packages it may take them a lot of effort. (I expect the transitive removal of `algorithm` and `type_traits` will uncover a lot of missing includes.) So I think keeping them when it doesn't "hurt" us might be even beneficial for us. If a vendor needs to fix a lot of packages or file a lot of bugs it will take them more effort to package, which may lead them to take a longer time to adopt a new libc++.
> > 
> > The policy still allows us the move things around when it "hurts" us. So if implementing PMR means we need to remove additional transitive includes we still should do that, but only try to minimize that. Like in this commit I removed `chrono` since it makes it hard to implement `format`.
> > 
> > The end goal is to move these transitive includes to the end of the header.
> > 
> > So I would really like to use this policy and when it turns out it doesn't work we can revise it. We explicitly mention we're free to remove them here and in the release notes.
> I get that it might be some effort for vendors to file bugs and stuff. That's why my proposal is to remove transitive includes at any point when there is a technical problem with keeping them. When we think we removed most/all the transitive includes we want to remove we tell everyone well in advance that we will remove the transitive includes in LLVM ab. If we think that we don't remove many after LLVM 16, we can say that transitive includes will be removed in LLVM 18 or whatever.
> 
> While this does result in some work IMO it's worth it. For example, including `<string>` already only takes 2/3 as long when adding `-D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to the command line and I'm certain this can be reduced further. Including `<memory>` currently adds about 100ms (the compilation took ~385ms).
Let's discuss this further on Discord.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132284