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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116146