[libcxx-commits] [PATCH] D116146: [libc++] Remove unused headers from <filesystem>

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/filesystem:258
 #include <compare>
-#include <cstddef>
-#include <cstdlib>
-#include <iosfwd>
-#include <iterator>
-#include <memory>
-#include <stack>
-#include <string>
-#include <string_view>
-#include <system_error>
-#include <utility>
 #include <version>
 
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > I'm fairly sure you don't need `<version>` in here. Compare to other granularized headers, such as `<compare>` and `<random>`, which don't include it.
> > > OTOH, `<concepts>`, `<coroutine>`, `<iterator>`, and `<utility>` do include it.
> > > I guess since we're inconsistent, no action is needed either way right now.
> > > I can make a followup PR to remove it from everywhere (or, if that fails tests and/or someone's reading of the Standard, to //add// it everywhere).
> > I think we need version and not depend on other headers providing it. `<version>` makes the feature-test macros available. Some macros are required for `<filesystem>`. For now `<compare>` is fine, until we finish `__cpp_lib_three_way_comparison`, then `<version>` is required. It seems `<random>` is fine not to include it. 
> > 
> > I'll provide a patch for `<compare>`.
> Ah, I (think I) see; you're saying that any top-level header that is mandated to provide a feature-test macro, must `#include <version>` because that's where libc++'s feature-test macros are centralized. I think that makes sense, yeah.
> 
> Several other top-level headers are missing `<version>`, then; e.g. `<barrier>` gets it transitively from `<atomic>`. I'd support a patch to add it to all of them... or, honestly, maybe we should just add `<version>` to `<__config>` or something crazy like that, so that we never have to think about it again.
> 
> ```
> $ grep -o '<[a-z_]*>' ../libcxx/include/version | sort | uniq | sed 's@<\(.*\)>@../libcxx/include/\1@' | xargs grep -L '<version>'
> ../libcxx/include/__config
> ../libcxx/include/barrier
> ../libcxx/include/charconv
> ../libcxx/include/compare
> ../libcxx/include/execution
> ../libcxx/include/latch
> grep: ../libcxx/include/memory_resource: No such file or directory
> ../libcxx/include/semaphore
> grep: ../libcxx/include/source_location: No such file or directory
> grep: ../libcxx/include/stacktrace: No such file or directory
> grep: ../libcxx/include/stop_token: No such file or directory
> grep: ../libcxx/include/syncstream: No such file or directory
> ../libcxx/include/thread
> ```
> 
Yes so libc++ also provides these macros in headers where they're not required. 
I don't think that's required since we have generated tests whether required feature-test macros are available. So marking `__cpp_lib_three_way_comparison` should fail during the build.

But I won't object against including `<version>` in `<__config>`, but I think it would be good to get @ldionne's opinion on that matter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116146