[libcxx-commits] [PATCH] D115578: [libc++][NFC] Granularize <filesystem>

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

This LGTM now, and we can try to clean up the unnecessary includes in `<filesystem>` in a separate patch.



================
Comment at: libcxx/include/filesystem:268
 #include <version>
-
-#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-# include <locale>
-# include <iomanip> // for quoted
-#endif
+#include <system_error>
 
----------------
Quuxplusone wrote:
> I see your repeated updates fiddling with `#include`s here, and I imagine they're to fix test failures with Modules? I believe that you should actually leave //all// of the public headers `#include`d here, i.e. please re-add `<chrono>` and `<cstddef>` and `<cstdlib>` and `<iosfwd>` and....
> (And re-alphabetize; you've got `<system_error>` below `<version>` right now.)
> 
> That will make this refactoring truly NFC.
> 
> Then, feel free to follow up with a PR to remove all these now-unused headers... but knowing that that will break anyone who's currently using `<filesystem>` with Modules and accidentally depending on these transitive includes, so it's not "NFC", it's just a good idea.  In that same PR, you would also update any libcxx/test/ files that are currently relying on these transitive includes to "Include What You Use".
Just to give additional background on this: people unfortunately often have missing includes in their code. As a result, they often rely on one of our transitive includes without realizing it, and we break a lot of code when we "clean up" our includes like that.

We're in favour of doing such clean up, however we need to be careful not to mix these changes with seemingly innocuous changes, otherwise things become much harder when shipping the library.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115578