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

Sat Aug 27 03:22:33 PDT 2022

Mordante marked 4 inline comments as done.
Mordante added a comment.

Thanks for all the feedback!

================
Comment at: libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst:11
+headers. For example, instead of including ``<algorithm>`` entirely it is
+possible to only include only the headers for the algorithms used. When the
+Standard indirectly adds additional header includes, using the smaller headers
----------------
philnik wrote:
> 
I agree with removing the `only` but the other part but the `the headers for` should remain.

================
Comment at: libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst:30
+To ease the removal of transitive includes in libc++, libc++ will remove
+unnecessary transitive includes in newly supported C++ versions. This means
+that users will have to fix their missing includes in order to upgrade to a
----------------
avogelsgesang wrote:
> when do we consider a C++ version as "supported"?
> As soon as we compile under that -std=c++XX flag without errors?
> As soon as clang flips from -std=c++2b to -std=c++23?
> As soon as we implemented all papers from that C++ version?
> 
> E.g., would we currently consider C++20 to be supported, despite not having implemented all papers, yet? If our bar is "some papers are implemented", which papers are those? Would we already consider C++23 as supported, given that we have implemented some of the C++23 papers?
C++20 is considered supported, that's why we put ranges and format under special flags.
I left supported here intentionally since it's already defined in another part of our policy.

https://libcxx.llvm.org/UsingLibcxx.html#using-a-different-version-of-the-c-standard

================
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:
> > > 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.

================
Comment at: libcxx/docs/ReleaseNotes.rst:62-64
+  in use. Note that in the future, libc++ reserves the right to remove
+  incidental transitive includes more aggressively, in particular regardless
+  of the language version in use.
----------------
mumbleskates wrote:
> Is "the future" here major releases? years? If this deeply affected me I would want to know a lot more about what kind of timeline I was guaranteed, given "regardless of the language version in use".
We kind of intentionally don't want to provide any guarantee. We still miss some C++17 features like parallel algorithms and polymorphic allocators. They may cause header cycles we want to break by removing headers.

Technically code not using the proper includes is wrong. But often you "get away" with it. But not including the proper headers makes code less portable. Some code accepted by libstdc++ will be rejected by libc++ since both libraries have different transitive includes. If you want to make sure your code doesn't miss any includes you can use `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to test before it becomes an issue.

Note that in the C++ Standard there are few required headers and some might even be surprising; `<regex>` doesn't require `<string>`. Based on the interface of `<regex>` I'm not convinced it can be implemented (in a practical way) without providing `<string>`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132284