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

[philnik]

Summary

Remove unused headers from <filesystem>

[• Quuxplusone]

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

240–257

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

259

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

259

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

[Mordante]

LGTM modulo the comments.

libcxx/include/filesystem

259

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

[• Quuxplusone]

libcxx/include/filesystem

259

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]

libcxx/include/filesystem

259

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.

[philnik]

This comment was removed by philnik.

[philnik]

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

[philnik]

• Add <compare>

[ldionne]

libcxx/include/filesystem

259

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.

[• Quuxplusone]

libcxx/include/filesystem

259

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]

libcxx/include/filesystem

259

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.

libcxx/include/filesystem

 inline constexpr bool std::ranges::enable_borrowed_range<std::filesystem::recursive_directory_iterator> = true;
 
 template <>
 inline constexpr bool std::ranges::enable_view<std::filesystem::directory_iterator> = true;
 template <>
 inline constexpr bool std::ranges::enable_view<std::filesystem::recursive_directory_iterator> = true;
 
 */
 
-#include<__filesystem/copy_options.h>
-#include<__filesystem/directory_entry.h>
-#include<__filesystem/directory_iterator.h>
-#include<__filesystem/directory_options.h>
-#include<__filesystem/file_status.h>
-#include<__filesystem/file_time_type.h>
-#include<__filesystem/file_type.h>
-#include<__filesystem/filesystem_error.h>
-#include<__filesystem/operations.h>
-#include<__filesystem/path_iterator.h>
-#include<__filesystem/path.h>
-#include<__filesystem/perm_options.h>
-#include<__filesystem/perms.h>
-#include<__filesystem/recursive_directory_iterator.h>
-#include<__filesystem/space_info.h>
-#include<__filesystem/u8path.h>
-
-#include <__availability>
-#include <__config>
-#include <__debug>
-#include <__ranges/enable_borrowed_range.h>
-#include <__ranges/enable_view.h>
-#include <__utility/forward.h>
-#include <chrono>
+#include <__config>
+#include <__filesystem/copy_options.h>
+#include <__filesystem/directory_entry.h>
+#include <__filesystem/directory_iterator.h>
+#include <__filesystem/directory_options.h>
+#include <__filesystem/file_status.h>
+#include <__filesystem/file_time_type.h>
+#include <__filesystem/file_type.h>
+#include <__filesystem/filesystem_error.h>
+#include <__filesystem/operations.h>
+#include <__filesystem/path_iterator.h>
+#include <__filesystem/path.h>
+#include <__filesystem/perm_options.h>
+#include <__filesystem/perms.h>
+#include <__filesystem/recursive_directory_iterator.h>
+#include <__filesystem/space_info.h>
+#include <__filesystem/u8path.h>
 #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>
 
-#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-# include <locale>
-# include <iomanip> // for quoted
-#endif
-
 #if defined(_LIBCPP_HAS_NO_FILESYSTEM_LIBRARY)
 # error "The Filesystem library is not supported since libc++ has been configured with LIBCXX_ENABLE_FILESYSTEM disabled"
 #endif
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 #endif // _LIBCPP_FILESYSTEM