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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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>
 
----------------
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
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116146