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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__functional_base:22-26
-#include <exception>
-#include <new>
-#include <type_traits>
-#include <typeinfo>
-#include <utility>
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119439