This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove unused headers from <filesystem>
ClosedPublic

Authored by philnik on Dec 22 2021, 2:02 AM.

Details

Diff Detail

Event Timeline

philnik requested review of this revision.Dec 22 2021, 2:02 AM
philnik created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2021, 2:02 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added a subscriber: Mordante.

This LGTM%comments. Please address comments, wait for a second libc++ approver (@Mordante ping?), and then when you land this, please land it in two git commits — one commit that does everything but the removal-of-lines, and then a second commit that does nothing but remove the lines. I think it's likely that this might break someone and then get reverted (especially if it breaks someone over the holiday weekend), so let's make sure that if (when) the removal commit is reverted, the rest of your work here will be in a different commit and thus remain unreverted.

libcxx/include/filesystem
256

All these lines should have a space between include and <.

258

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).

259

Please leave <__config>, for consistency with all libc++ headers; but move it into alphabetical order above <__fil....

Mordante accepted this revision.Dec 22 2021, 9:13 AM

LGTM modulo the comments.

libcxx/include/filesystem
258

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

This revision is now accepted and ready to land.Dec 22 2021, 9:13 AM
libcxx/include/filesystem
258

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
Mordante added inline comments.Dec 22 2021, 10:08 AM
libcxx/include/filesystem
258

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.

This revision was automatically updated to reflect the committed changes.
This comment was removed by philnik.
philnik reopened this revision.Dec 23 2021, 3:03 AM
This revision is now accepted and ready to land.Dec 23 2021, 3:03 AM
philnik updated this revision to Diff 396003.Dec 23 2021, 3:20 AM
philnik marked 5 inline comments as done.

This PR should now only be the removed headers in <filesystem>

philnik updated this revision to Diff 396004.Dec 23 2021, 3:22 AM
  • Add <compare>
This revision was automatically updated to reflect the committed changes.
ldionne added inline comments.Dec 29 2021, 6:13 AM
libcxx/include/filesystem
258

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.

libcxx/include/filesystem
258

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." ;)

Mordante added inline comments.Dec 29 2021, 9:45 AM
libcxx/include/filesystem
258

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.