[libcxx-commits] [PATCH] D119439: [libc++] Remove __functional_base

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision as: ldionne.
ldionne added a comment.

I'm fine with this in essence, and I'm looking to a follow-up PR where we remove unneeded includes (from the headers in this patch but also from other places, I'm sure we have a bunch).



================
Comment at: libcxx/include/__functional_base:22-26
-#include <exception>
-#include <new>
-#include <type_traits>
-#include <typeinfo>
-#include <utility>
----------------
Mordante wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > If you're marking this as NFC, then please make sure that each top-level header that //previously// transitively included `<__functional_base>` still (directly) includes each of these headers. E.g. if some user is doing
> > > ```
> > > #include <bitset>
> > > auto x = typeid(int);  // depends on <typeinfo>
> > > ```
> > > we don't want to break them. You can mark any `#include` lines added in this way with a comment like `// TODO: remove this`.
> > > 
> > > IMHO we don't need to add direct inclusions to `<format>` or `<ranges>`, but I'd recommend doing it for all the others. (And by doing it for `<iterator>`, you'll pick up `<ranges>` for free anyway.)
> > This ^
> > 
> > Removing includes is a very common source of breakage. Unfortunately, people don't include what they use all the time, and as a result they can be broken when we remove a transitive include. Removing headers is OK, but it has to be called out explicitly and release-noted, and concretely we have to be aware that it's going to trigger some work on the vendor side (they will see failures downstream when rebuilding code and they will have to fix the projects that didn't have the correct includes).
> > 
> > Ideally, this commit would remove `__functional_base` while still making sure we include all the stuff we used to include. And in a separate commit, we can remove those includes and release-note it. I can also run a build and try to see what the impact of the removal would be.
> Wouldn't it be easier to remove all includes from `__functional_base` and then remove it in a separate commit?
> +1 for adding this to the release notes.
> 
> (A slight side note, but maybe it's a good time to try to remove some of the unneeded transitive includes in this release cycle. That might break more, but then it's a one time pain to fix it for users. The upside is that compilation will be faster when not everything includes all of `<algorithm>`. I will volunteer to do the cleanup.)
I agree -- if we want to remove unused transitive includes, it would be a pretty good time in the cycle to do it. As far as I'm concerned, this will leave plenty of time for vendors to shake out issues on their end before we cut the release. And if things break in unexpected and bad ways, we can always revert or tweak.


================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/iterator/minus.pass.cpp:20
+#include <cstdint>
+#include <ranges>
 
----------------
philnik wrote:
> Quuxplusone wrote:
> > Please leave `<ranges>` first; historically this is our style for tests that test a particular header (although I admit I don't see much point to it).
> I didn't know that rule and nobody has said anything in my other PRs until now, so I'd like to know @ldionne's opinion on this. I don't see much point to it either.
That's true, we've always done this (rather blindly TBH). We may not have noticed on your other PRs.

The benefit of having the header first is that we make sure the header can be included when nothing else has been included before. I'm fine with not doing this anymore in our tests, however I think it would be nice to also have generated tests that simply include every single public header in an otherwise empty translation unit, just to make sure it works.

TLDR: Let's test this properly and then we can safely drop this weird custom we have. I'm fine with this patch not following the usual rule.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119439