[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:
> ldionne wrote:
> > Mordante wrote:
> > > 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.
> > I think we should include `<version>` in any header that needs to provide feature-test macros. However, `__config` is kind of special in that it is an internal header, and we want to avoid including any other headers from that one if we can. So I'd be against including `<version>` from `<__config>` just for convenience.
> FWIW, I'm pretty sure that "just add `<version>` to `<__config>` or something crazy like that" would be slightly more of a research project than //just// adding `#include <version>` in `<__config>` (because I assume that would just create a circular dependency). My idea was just that we'd come out the other side of the project with some simple "boilerplate header" (maybe also including `<__availability>`) that could be safely included in every top-level header, probably //including// `<version>`.
> 
> Anyway, "include `<version>` everywhere" is D116172, just waiting on its second accepter. I'm happier with the rule being "include it everywhere" than with "include it only some places, and you have to consult the standard to figure out which places those are this year." ;)
> Anyway, "include `<version>` everywhere" is D116172, just waiting on its second accepter. I'm happier with the rule being "include it everywhere" than with "include it only some places, and you have to consult the standard to figure out which places those are this year." ;)

I'm waiting on Buildkite to complete its build ;-) It's quite trivial and shouldn't fail, but I'm not in a hurry to land it.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116146