[Support] Listing a directory containing dangling symlinks is not an …

…error.

Summary:
Reporting this as an error required stat()ing every file, as well as seeming
semantically questionable.

Reviewers: vsk, bkramer

Subscribers: mgrang, kristina, llvm-commits, liaoyuke

Differential Revision: https://reviews.llvm.org/D52648

llvm-svn: 343460

llvm/include/llvm/Support/FileSystem.h

@@ -1182,7 +1182,6 @@ class directory_iterator {
     SmallString<128> path_storage;
     ec = detail::directory_iterator_construct(
         *State, path.toStringRef(path_storage), FollowSymlinks);
-    update_error_code_for_current_entry(ec);
   }
 
   explicit directory_iterator(const directory_entry &de, std::error_code &ec,
@@ -1191,7 +1190,6 @@ class directory_iterator {
     State = std::make_shared<detail::DirIterState>();
     ec = detail::directory_iterator_construct(
         *State, de.path(), FollowSymlinks);
-    update_error_code_for_current_entry(ec);
   }
 
   /// Construct end iterator.
@@ -1200,7 +1198,6 @@ class directory_iterator {
   // No operator++ because we need error_code.
   directory_iterator &increment(std::error_code &ec) {
     ec = directory_iterator_increment(*State);
-    update_error_code_for_current_entry(ec);
     return *this;
   }
 
@@ -1220,26 +1217,6 @@ class directory_iterator {
   bool operator!=(const directory_iterator &RHS) const {
     return !(*this == RHS);
   }
-  // Other members as required by
-  // C++ Std, 24.1.1 Input iterators [input.iterators]
-
-private:
-  // Checks if current entry is valid and populates error code. For example,
-  // current entry may not exist due to broken symbol links.
-  void update_error_code_for_current_entry(std::error_code &ec) {
-    // Bail out if error has already occured earlier to avoid overwriting it.
-    if (ec)
-      return;
-
-    // Empty directory entry is used to mark the end of an interation, it's not
-    // an error.
-    if (State->CurrentEntry == directory_entry())
-      return;
-
-    ErrorOr<basic_file_status> status = State->CurrentEntry.status();
-    if (!status)
-      ec = status.getError();
-  }
 };
 
 namespace detail {
@@ -1277,8 +1254,15 @@ class recursive_directory_iterator {
     if (State->HasNoPushRequest)
       State->HasNoPushRequest = false;
     else {
-      ErrorOr<basic_file_status> status = State->Stack.top()->status();
-      if (status && is_directory(*status)) {
+      file_type type = State->Stack.top()->type();
+      if (type == file_type::symlink_file && Follow) {
+        // Resolve the symlink: is it a directory to recurse into?
+        ErrorOr<basic_file_status> status = State->Stack.top()->status();
+        if (status)
+          type = status->type();
+        // Otherwise broken symlink, and we'll continue.
+      }
+      if (type == file_type::directory_file) {
         State->Stack.push(directory_iterator(*State->Stack.top(), ec, Follow));
         if (State->Stack.top() != end_itr) {
           ++State->Level;
@@ -1342,8 +1326,6 @@ class recursive_directory_iterator {
   bool operator!=(const recursive_directory_iterator &RHS) const {
     return !(*this == RHS);
   }
-  // Other members as required by
-  // C++ Std, 24.1.1 Input iterators [input.iterators]
 };
 
 /// @}

llvm/tools/llvm-cov/CodeCoverage.cpp

@@ -215,12 +215,13 @@ void CodeCoverageTool::collectPaths(const std::string &Path) {
     for (llvm::sys::fs::recursive_directory_iterator F(Path, EC), E;
          F != E; F.increment(EC)) {
 
-      if (EC) {
-        warning(EC.message(), F->path());
+      auto Status = F->status();
+      if (!Status) {
+        warning(Status.getError().message(), F->path());
         continue;
       }
 
-      if (llvm::sys::fs::is_regular_file(F->path()))
+      if (Status->type() == llvm::sys::fs::file_type::regular_file)
         addCollectedPath(F->path());
     }
   }

llvm/unittests/Support/Path.cpp

@@ -883,87 +883,62 @@ TEST_F(FileSystemTest, BrokenSymlinkDirectoryIteration) {
   v_t VisitedNonBrokenSymlinks;
   v_t VisitedBrokenSymlinks;
   std::error_code ec;
+  using testing::UnorderedElementsAre;
+  using testing::UnorderedElementsAreArray;
 
   // Broken symbol links are expected to throw an error.
   for (fs::directory_iterator i(Twine(TestDirectory) + "/symlink", ec), e;
        i != e; i.increment(ec)) {
-    if (ec == std::make_error_code(std::errc::no_such_file_or_directory)) {
+    ASSERT_NO_ERROR(ec);
+    if (i->status().getError() ==
+        std::make_error_code(std::errc::no_such_file_or_directory)) {
       VisitedBrokenSymlinks.push_back(path::filename(i->path()));
       continue;
     }
-
-    ASSERT_NO_ERROR(ec);
     VisitedNonBrokenSymlinks.push_back(path::filename(i->path()));
   }
-  llvm::sort(VisitedNonBrokenSymlinks);
-  llvm::sort(VisitedBrokenSymlinks);
-  v_t ExpectedNonBrokenSymlinks = {"b", "d"};
-  ASSERT_EQ(ExpectedNonBrokenSymlinks.size(), VisitedNonBrokenSymlinks.size());
-  ASSERT_TRUE(std::equal(VisitedNonBrokenSymlinks.begin(),
-                         VisitedNonBrokenSymlinks.end(),
-                         ExpectedNonBrokenSymlinks.begin()));
+  EXPECT_THAT(VisitedNonBrokenSymlinks, UnorderedElementsAre("b", "d"));
   VisitedNonBrokenSymlinks.clear();
 
-  v_t ExpectedBrokenSymlinks = {"a", "c", "e"};
-  ASSERT_EQ(ExpectedBrokenSymlinks.size(), VisitedBrokenSymlinks.size());
-  ASSERT_TRUE(std::equal(VisitedBrokenSymlinks.begin(),
-                         VisitedBrokenSymlinks.end(),
-                         ExpectedBrokenSymlinks.begin()));
+  EXPECT_THAT(VisitedBrokenSymlinks, UnorderedElementsAre("a", "c", "e"));
   VisitedBrokenSymlinks.clear();
 
   // Broken symbol links are expected to throw an error.
   for (fs::recursive_directory_iterator i(
       Twine(TestDirectory) + "/symlink", ec), e; i != e; i.increment(ec)) {
-    if (ec == std::make_error_code(std::errc::no_such_file_or_directory)) {
+    ASSERT_NO_ERROR(ec);
+    if (i->status().getError() ==
+        std::make_error_code(std::errc::no_such_file_or_directory)) {
       VisitedBrokenSymlinks.push_back(path::filename(i->path()));
       continue;
     }
-
-    ASSERT_NO_ERROR(ec);
     VisitedNonBrokenSymlinks.push_back(path::filename(i->path()));
   }
-  llvm::sort(VisitedNonBrokenSymlinks);
-  llvm::sort(VisitedBrokenSymlinks);
-  ExpectedNonBrokenSymlinks = {"b", "bb", "d", "da", "dd", "ddd", "ddd"};
-  ASSERT_EQ(ExpectedNonBrokenSymlinks.size(), VisitedNonBrokenSymlinks.size());
-  ASSERT_TRUE(std::equal(VisitedNonBrokenSymlinks.begin(),
-                         VisitedNonBrokenSymlinks.end(),
-                         ExpectedNonBrokenSymlinks.begin()));
+  EXPECT_THAT(VisitedNonBrokenSymlinks,
+              UnorderedElementsAre("b", "bb", "d", "da", "dd", "ddd", "ddd"));
   VisitedNonBrokenSymlinks.clear();
 
-  ExpectedBrokenSymlinks = {"a", "ba", "bc", "c", "e"};
-  ASSERT_EQ(ExpectedBrokenSymlinks.size(), VisitedBrokenSymlinks.size());
-  ASSERT_TRUE(std::equal(VisitedBrokenSymlinks.begin(),
-                         VisitedBrokenSymlinks.end(),
-                         ExpectedBrokenSymlinks.begin()));
+  EXPECT_THAT(VisitedBrokenSymlinks,
+              UnorderedElementsAre("a", "ba", "bc", "c", "e"));
   VisitedBrokenSymlinks.clear();
 
   for (fs::recursive_directory_iterator i(
       Twine(TestDirectory) + "/symlink", ec, /*follow_symlinks=*/false), e;
        i != e; i.increment(ec)) {
-    if (ec == std::make_error_code(std::errc::no_such_file_or_directory)) {
+    ASSERT_NO_ERROR(ec);
+    if (i->status().getError() ==
+        std::make_error_code(std::errc::no_such_file_or_directory)) {
       VisitedBrokenSymlinks.push_back(path::filename(i->path()));
       continue;
     }
-
-    ASSERT_NO_ERROR(ec);
     VisitedNonBrokenSymlinks.push_back(path::filename(i->path()));
   }
-  llvm::sort(VisitedNonBrokenSymlinks);
-  llvm::sort(VisitedBrokenSymlinks);
-  ExpectedNonBrokenSymlinks = {"a", "b", "ba", "bb", "bc", "c", "d", "da", "dd",
-                               "ddd", "e"};
-  ASSERT_EQ(ExpectedNonBrokenSymlinks.size(), VisitedNonBrokenSymlinks.size());
-  ASSERT_TRUE(std::equal(VisitedNonBrokenSymlinks.begin(),
-                         VisitedNonBrokenSymlinks.end(),
-                         ExpectedNonBrokenSymlinks.begin()));
+  EXPECT_THAT(VisitedNonBrokenSymlinks,
+              UnorderedElementsAreArray({"a", "b", "ba", "bb", "bc", "c", "d",
+                                         "da", "dd", "ddd", "e"}));
   VisitedNonBrokenSymlinks.clear();
 
-  ExpectedBrokenSymlinks = {};
-  ASSERT_EQ(ExpectedBrokenSymlinks.size(), VisitedBrokenSymlinks.size());
-  ASSERT_TRUE(std::equal(VisitedBrokenSymlinks.begin(),
-                         VisitedBrokenSymlinks.end(),
-                         ExpectedBrokenSymlinks.begin()));
+  EXPECT_THAT(VisitedBrokenSymlinks, UnorderedElementsAre());
   VisitedBrokenSymlinks.clear();
 
   ASSERT_NO_ERROR(fs::remove_directories(Twine(TestDirectory) + "/symlink"));