[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

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]

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

[• Quuxplusone]

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]

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.

[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

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.

[• Quuxplusone]

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]

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.

libcxx/include/filesystem

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

libcxx/src/filesystem/directory_iterator.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-#include "filesystem"
 #include "__config"
+#include "filesystem"
+#include "stack"
 #if defined(_LIBCPP_WIN32API)
 #define WIN32_LEAN_AND_MEAN
 #define NOMINMAX
 #include <windows.h>
 #else
 #include <dirent.h>
 #endif
 #include <errno.h>

libcxx/src/filesystem/filesystem_common.h

 #include "__config"
 #include "array"
 #include "chrono"
 #include "climits"
 #include "cstdarg"
 #include "cstdlib"
 #include "ctime"
 #include "filesystem"
+#include "system_error"
 
 #if !defined(_LIBCPP_WIN32API)
 # include <unistd.h>
 # include <sys/stat.h>
 # include <sys/statvfs.h>
 # include <sys/time.h> // for ::utimes as used in __last_write_time
 # include <fcntl.h>    /* values for fchmodat */
 #endif

libcxx/test/libcxx/input.output/filesystems/convert_file_time.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11
 
 // <filesystem>
 
 // typedef TrivialClock file_time_type;
 
 // ADDITIONAL_COMPILE_FLAGS: -I %S/../../../../src/filesystem
 
-#include <filesystem>
+#include <cassert>
 #include <chrono>
-#include <type_traits>
-#include <limits>
 #include <cstddef>
-#include <cassert>
+#include <filesystem>
+#include <limits>
+#include <string>
+#include <type_traits>
 
 #include "filesystem_common.h"
 
 #ifndef __SIZEOF_INT128__
 #define TEST_HAS_NO_INT128_T
 #endif
 
 using namespace std::chrono;

libcxx/test/std/input.output/filesystems/class.directory_entry/directory_entry.obs/comparisons.pass.cpp

 
 // bool operator==(directory_entry const&) const noexcept;
 // bool operator!=(directory_entry const&) const noexcept;
 // bool operator< (directory_entry const&) const noexcept;
 // bool operator<=(directory_entry const&) const noexcept;
 // bool operator> (directory_entry const&) const noexcept;
 // bool operator>=(directory_entry const&) const noexcept;
 
-
 #include "filesystem_include.h"
-#include <type_traits>
 #include <cassert>
+#include <type_traits>
+#include <utility>
 
 #include "test_macros.h"
 
 
 #define CHECK_OP(Op) \
   static_assert(std::is_same<decltype(ce Op ce), bool>::value, ""); \
   static_assert(noexcept(ce Op ce), "Operation must be noexcept" )
 

libcxx/test/std/input.output/filesystems/class.directory_iterator/types.pass.cpp

 
 // typedef ... value_type;
 // typedef ... difference_type;
 // typedef ... pointer;
 // typedef ... reference;
 // typedef ... iterator_category
 
 #include "filesystem_include.h"
-#include <type_traits>
 #include <cassert>
+#include <iterator>
+#include <type_traits>
 
 #include "test_macros.h"
 
 
 int main(int, char**) {
     using namespace fs;
     using D = directory_iterator;
     ASSERT_SAME_TYPE(D::value_type, directory_entry);
     ASSERT_SAME_TYPE(D::difference_type, std::ptrdiff_t);
     ASSERT_SAME_TYPE(D::pointer, const directory_entry*);
     ASSERT_SAME_TYPE(D::reference, const directory_entry&);
     ASSERT_SAME_TYPE(D::iterator_category, std::input_iterator_tag);
 
   return 0;
 }

libcxx/test/std/input.output/filesystems/class.filesystem_error/filesystem_error.members.pass.cpp

 // filesystem_error(const string& what_arg, const path& p1, error_code ec);
 // filesystem_error(const string& what_arg, const path& p1, const path& p2, error_code ec);
 // const std::error_code& code() const;
 // const char* what() const noexcept;
 // const path& path1() const noexcept;
 // const path& path2() const noexcept;
 
 #include "filesystem_include.h"
-#include <type_traits>
 #include <cassert>
+#include <string>
+#include <system_error>
+#include <type_traits>
 
 #include "test_macros.h"
 
 
 void test_constructors() {
   using namespace fs;
 
   // The string returned by "filesystem_error::what() must contain runtime_error::what()

libcxx/test/std/input.output/filesystems/class.path/path.member/path.assign/copy.pass.cpp

 
 // <filesystem>
 
 // class path
 
 // path& operator=(path const&);
 
 #include "filesystem_include.h"
-#include <type_traits>
 #include <cassert>
+#include <string>
+#include <type_traits>
 
 #include "test_macros.h"
 
 
 int main(int, char**) {
   using namespace fs;
   static_assert(std::is_copy_assignable<path>::value, "");
   static_assert(!std::is_nothrow_copy_assignable<path>::value, "should not be noexcept");

libcxx/test/std/input.output/filesystems/class.path/path.member/path.assign/move.pass.cpp

 
 // <filesystem>
 
 // class path
 
 // path& operator=(path&&) noexcept
 
 #include "filesystem_include.h"
-#include <type_traits>
 #include <cassert>
+#include <string>
+#include <type_traits>
 
 #include "test_macros.h"
 #include "count_new.h"
 
 
 int main(int, char**) {
   using namespace fs;
   static_assert(std::is_nothrow_move_assignable<path>::value, "");

libcxx/test/std/input.output/filesystems/class.path/path.member/path.construct/copy.pass.cpp

 
 // <filesystem>
 
 // class path
 
 // path(path const&)
 
 #include "filesystem_include.h"
-#include <type_traits>
 #include <cassert>
+#include <string>
+#include <type_traits>
 
 #include "test_macros.h"
 
 
 int main(int, char**) {
   using namespace fs;
   static_assert(std::is_copy_constructible<path>::value, "");
   static_assert(!std::is_nothrow_copy_constructible<path>::value, "should not be noexcept");
   const std::string s("foo");
   const path p(s);
   path p2(p);
   assert(p.string() == s);
   assert(p2.string() == s);
 
   return 0;
 }

libcxx/test/std/input.output/filesystems/class.path/path.member/path.construct/move.pass.cpp

 
 // <filesystem>
 
 // class path
 
 // path(path&&) noexcept
 
 #include "filesystem_include.h"
-#include <type_traits>
 #include <cassert>
+#include <string>
+#include <type_traits>
 
 #include "test_macros.h"
 #include "count_new.h"
 
 
 int main(int, char**) {
   using namespace fs;
   static_assert(std::is_nothrow_move_constructible<path>::value, "");

libcxx/test/std/input.output/filesystems/class.path/synop.pass.cpp

 
 // class path
 
 // typedef ... value_type;
 // typedef basic_string<value_type> string_type;
 // static constexpr value_type preferred_separator = ...;
 
 #include "filesystem_include.h"
-#include <type_traits>
 #include <cassert>
+#include <string>
+#include <type_traits>
 
 #include "test_macros.h"
 
 
 int main(int, char**) {
   using namespace fs;
 #ifdef _WIN32
   ASSERT_SAME_TYPE(path::value_type, wchar_t);

libcxx/test/support/filesystem_test_helper.h

 #ifndef FILESYSTEM_TEST_HELPER_H
 #define FILESYSTEM_TEST_HELPER_H
 
 #include "filesystem_include.h"
 
 #include <sys/stat.h> // for stat, mkdir, mkfifo
 #ifndef _WIN32
 #include <unistd.h> // for ftruncate, link, symlink, getcwd, chdir
 #include <sys/statvfs.h>
 #else
 #include <io.h>
 #include <direct.h>
 #include <windows.h> // for CreateSymbolicLink, CreateHardLink
 #endif
 
 #include <cassert>
+#include <chrono>
 #include <cstdio> // for printf
 #include <string>
-#include <chrono>
+#include <system_error>
 #include <vector>
 
 #include "make_string.h"
 #include "test_macros.h"
 #include "rapid-cxx-test.h"
 #include "format_string.h"
 
 // For creating socket files