[libcxx] Fixes after review, add & improve test.

Author: ed-sat

libcxx/include/__configuration/availability.h

@@ -353,9 +353,10 @@
 #define _LIBCPP_AVAILABILITY_HAS_BAD_EXPECTED_ACCESS_KEY_FUNCTION _LIBCPP_INTRODUCED_IN_LLVM_19
 #define _LIBCPP_AVAILABILITY_BAD_EXPECTED_ACCESS_KEY_FUNCTION _LIBCPP_INTRODUCED_IN_LLVM_19_ATTRIBUTE
 
-// This controls the availability of populating cache for directory entry
-// filesystem system. A directory_iterator builds cache for directory_entry
-// in the library.
+// This determines whether std::filesystem::directory_entry caches all the properties
+// it contains, which improves the performance when accessing a directory entry.
+// Code in the headers like filesystem::directory_entry's implementation needs to
+// know whether the dylib populated the cache or not.
 #define _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY _LIBCPP_INTRODUCED_IN_LLVM_19
 #define _LIBCPP_AVAILABILITY_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY _LIBCPP_INTRODUCED_IN_LLVM_19_ATTRIBUTE
 

libcxx/include/__filesystem/directory_entry.h

@@ -202,10 +202,8 @@ class directory_entry {
     _RefreshSymlink,
     _RefreshSymlinkUnresolved,
     _RefreshNonSymlink,
-#  if _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
     _IterCachedSymlink,
     _IterCachedNonSymlink
-#  endif // _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
   };
 
   struct __cached_data {
@@ -244,7 +242,6 @@ class directory_entry {
     return __data;
   }
 
-  #  if _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
   _LIBCPP_HIDE_FROM_ABI static __cached_data
   __create_iter_cached_result(file_type __ft, uintmax_t __size, perms __perm, file_time_type __write_time) {
     __cached_data __data;
@@ -267,7 +264,6 @@ class directory_entry {
     }();
     return __data;
   }
-  #  endif // _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
 
   _LIBCPP_HIDE_FROM_ABI void __assign_iter_entry(_Path&& __p, __cached_data __dt) {
     __p_    = std::move(__p);
@@ -310,17 +306,13 @@ class directory_entry {
     case _Empty:
       return __symlink_status(__p_, __ec).type();
     case _IterSymlink:
-#  if _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
     case _IterCachedSymlink:
-#  endif // _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
     case _RefreshSymlink:
     case _RefreshSymlinkUnresolved:
       if (__ec)
         __ec->clear();
       return file_type::symlink;
-#  if _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
     case _IterCachedNonSymlink:
-#  endif // _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
     case _IterNonSymlink:
     case _RefreshNonSymlink: {
       file_status __st(__data_.__type_);
@@ -338,13 +330,10 @@ class directory_entry {
     switch (__data_.__cache_type_) {
     case _Empty:
     case _IterSymlink:
-    case _RefreshSymlinkUnresolved:
-      return __status(__p_, __ec).type();
-#  if _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
     case _IterCachedSymlink:
+    case _RefreshSymlinkUnresolved:
       return __status(__p_, __ec).type();
     case _IterCachedNonSymlink:
-#  endif // _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
     case _IterNonSymlink:
     case _RefreshNonSymlink:
     case _RefreshSymlink: {
@@ -364,17 +353,13 @@ class directory_entry {
     case _Empty:
     case _IterNonSymlink:
     case _IterSymlink:
+    case _IterCachedSymlink:
     case _RefreshSymlinkUnresolved:
       return __status(__p_, __ec);
+    case _IterCachedNonSymlink:
     case _RefreshNonSymlink:
     case _RefreshSymlink:
       return file_status(__get_ft(__ec), __data_.__non_sym_perms_);
-#    if _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
-    case _IterCachedSymlink:
-      return __status(__p_, __ec);
-    case _IterCachedNonSymlink:
-      return file_status(__get_ft(__ec), __data_.__non_sym_perms_);
-#    endif // _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
     }
     __libcpp_unreachable();
   }
@@ -385,17 +370,13 @@ class directory_entry {
     case _IterNonSymlink:
     case _IterSymlink:
       return __symlink_status(__p_, __ec);
+    case _IterCachedNonSymlink:
     case _RefreshNonSymlink:
       return file_status(__get_sym_ft(__ec), __data_.__non_sym_perms_);
+    case _IterCachedSymlink:
     case _RefreshSymlink:
     case _RefreshSymlinkUnresolved:
       return file_status(__get_sym_ft(__ec), __data_.__sym_perms_);
-#    if _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
-    case _IterCachedNonSymlink:
-      return file_status(__get_sym_ft(__ec), __data_.__non_sym_perms_);
-    case _IterCachedSymlink:
-      return file_status(__get_sym_ft(__ec), __data_.__sym_perms_);
-#    endif // _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
     }
     __libcpp_unreachable();
   }
@@ -405,13 +386,10 @@ class directory_entry {
     case _Empty:
     case _IterNonSymlink:
     case _IterSymlink:
-    case _RefreshSymlinkUnresolved:
-      return filesystem::__file_size(__p_, __ec);
-#  if _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
     case _IterCachedSymlink:
+    case _RefreshSymlinkUnresolved:
       return filesystem::__file_size(__p_, __ec);
     case _IterCachedNonSymlink:
-#  endif // _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
     case _RefreshSymlink:
     case _RefreshNonSymlink: {
       error_code __m_ec;
@@ -432,10 +410,8 @@ class directory_entry {
     case _Empty:
     case _IterNonSymlink:
     case _IterSymlink:
-#  if _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
     case _IterCachedNonSymlink:
     case _IterCachedSymlink:
-#  endif // _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
     case _RefreshSymlinkUnresolved:
       return filesystem::__hard_link_count(__p_, __ec);
     case _RefreshSymlink:
@@ -454,13 +430,10 @@ class directory_entry {
     case _Empty:
     case _IterNonSymlink:
     case _IterSymlink:
-    case _RefreshSymlinkUnresolved:
-      return filesystem::__last_write_time(__p_, __ec);
-#  if _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
     case _IterCachedSymlink:
+    case _RefreshSymlinkUnresolved:
       return filesystem::__last_write_time(__p_, __ec);
     case _IterCachedNonSymlink:
-#  endif // _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
     case _RefreshSymlink:
     case _RefreshNonSymlink: {
       error_code __m_ec;

libcxx/src/filesystem/directory_iterator.cpp

@@ -79,16 +79,11 @@ class __dir_stream {
       return false;
     __entry_.__assign_iter_entry(
         __root_ / __data_.cFileName,
-#  if _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
         directory_entry::__create_iter_cached_result(
             detail::get_file_type(__data_),
             detail::get_file_size(__data_),
             detail::get_file_perm(__data_),
-            detail::get_write_time(__data_))
-#  else
-        directory_entry::__create_iter_result(detail::get_file_type(__data_))
-#  endif // _LIBCPP_AVAILABILITY_HAS_FILESYSTEM_FULLY_POPULATED_CACHED_ENTRY
-    );
+            detail::get_write_time(__data_)));
     return true;
   }
 

libcxx/src/filesystem/file_descriptor.h

@@ -99,7 +99,7 @@ inline uintmax_t get_file_size(const WIN32_FIND_DATAW& data) {
 inline file_time_type get_write_time(const WIN32_FIND_DATAW& data) {
   using detail::fs_time;
   const FILETIME& time = data.ftLastWriteTime;
-  auto ts = filetime_to_timespec(time);
+  auto ts              = filetime_to_timespec(time);
   if (!fs_time::is_representable(ts))
     return file_time_type::min();
   return fs_time::convert_from_timespec(ts);

libcxx/test/std/input.output/filesystems/class.rec.dir.itr/cache_refresh_iter.pass.cpp

@@ -32,39 +32,81 @@ static void set_last_write_time_in_iteration(const fs::path& dir) {
   // See
   // https://learn.microsoft.com/en-us/windows/win32/sysinfo/file-times
   // To force updating file entries calls "last_write_time" with own value.
-  const recursive_directory_iterator endIt{};
+  const recursive_directory_iterator end_it{};
 
   std::error_code ec;
   recursive_directory_iterator it(dir, ec);
   assert(!ec);
 
   file_time_type now_time = file_time_type::clock::now();
-  for (; it != endIt; ++it) {
+  for (; it != end_it; ++it) {
     const path entry = *it;
     last_write_time(entry, now_time, ec);
     assert(!ec);
   }
 
-  assert(it == endIt);
+  assert(it == end_it);
+}
+
+struct directory_entry_and_values {
+  directory_entry entry;
+
+  file_status symlink_status;
+  file_status status;
+  std::uintmax_t file_size;
+  file_time_type last_write_time;
+};
+
+std::vector<directory_entry_and_values> get_directory_entries_for(const path& dir, const std::set<path>& dir_contents) {
+  const recursive_directory_iterator end_it{};
+
+  std::error_code ec;
+  recursive_directory_iterator it(dir, ec);
+  assert(!ec);
+
+  std::vector<directory_entry_and_values> dir_entries;
+  std::set<path> unseen_entries = dir_contents;
+  while (!unseen_entries.empty()) {
+    assert(it != end_it);
+    const directory_entry& entry = *it;
+
+    assert(unseen_entries.erase(entry.path()) == 1);
+
+    dir_entries.push_back(directory_entry_and_values{
+        .entry           = entry,
+        .symlink_status  = entry.symlink_status(),
+        .status          = entry.status(),
+        .file_size       = entry.is_regular_file() ? entry.file_size() : 0,
+        .last_write_time = entry.last_write_time()});
+
+    recursive_directory_iterator& it_ref = it.increment(ec);
+    assert(!ec);
+    assert(&it_ref == &it);
+  }
+  return dir_entries;
 }
 #endif
 
+// Checks that the directory_entry properties will be the same before and after
+// calling "refresh" in case of iteration.
+// In case of Windows expects that directory_entry caches the properties during
+// iteration.
 static void test_cache_and_refresh_in_iteration() {
   static_test_env static_env;
-  const path testDir = static_env.Dir;
+  const path test_dir = static_env.Dir;
 #if defined(_WIN32)
-  set_last_write_time_in_iteration(testDir);
+  set_last_write_time_in_iteration(test_dir);
 #endif
   const std::set<path> dir_contents(static_env.RecDirIterationList.begin(), static_env.RecDirIterationList.end());
-  const recursive_directory_iterator endIt{};
+  const recursive_directory_iterator end_it{};
 
   std::error_code ec;
-  recursive_directory_iterator it(testDir, ec);
+  recursive_directory_iterator it(test_dir, ec);
   assert(!ec);
 
   std::set<path> unseen_entries = dir_contents;
   while (!unseen_entries.empty()) {
-    assert(it != endIt);
+    assert(it != end_it);
     const directory_entry& entry = *it;
 
     assert(unseen_entries.erase(entry.path()) == 1);
@@ -92,8 +134,54 @@ static void test_cache_and_refresh_in_iteration() {
   }
 }
 
+#if defined(_WIN32)
+// In case of Windows expects that the directory_entry caches the properties
+// during iteration and the properties don't change after deleting folders
+// and files.
+static void test_cached_values_in_iteration() {
+  std::vector<directory_entry_and_values> dir_entries;
+  {
+    static_test_env static_env;
+    const path testDir = static_env.Dir;
+    set_last_write_time_in_iteration(testDir);
+    const std::set<path> dir_contents(static_env.RecDirIterationList.begin(), static_env.RecDirIterationList.end());
+    dir_entries = get_directory_entries_for(testDir, dir_contents);
+  }
+  // Testing folder should be deleted after destoying static_test_env.
+
+  for (const auto& dir_entry : dir_entries) {
+    // During iteration Windows provides information only about symlink itself
+    // not about file/folder which symlink points to.
+    if (dir_entry.entry.is_symlink()) {
+      // Check that symlink is not using cached value about existing file.
+      assert(!dir_entry.entry.exists());
+    } else {
+      // Check that entry uses cached value about existing file.
+      assert(dir_entry.entry.exists());
+    }
+    file_status symlink_status = dir_entry.entry.symlink_status();
+    assert(dir_entry.symlink_status.type() == symlink_status.type() &&
+           dir_entry.symlink_status.permissions() == symlink_status.permissions());
+
+    if (!dir_entry.entry.is_symlink()) {
+      file_status status = dir_entry.entry.status();
+      assert(dir_entry.status.type() == status.type() && dir_entry.status.permissions() == status.permissions());
+
+      std::uintmax_t file_size = dir_entry.entry.is_regular_file() ? dir_entry.entry.file_size() : 0;
+      assert(dir_entry.file_size == file_size);
+
+      file_time_type last_write_time = dir_entry.entry.last_write_time();
+      assert(dir_entry.last_write_time == last_write_time);
+    }
+  }
+}
+#endif
+
 int main(int, char**) {
   test_cache_and_refresh_in_iteration();
+#if defined(_WIN32)
+  test_cached_values_in_iteration();
+#endif
 
   return 0;
 }