[PATCH] D44960: Prevent llvm-cov from hanging when a symblink doesn't exist.

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

This is looking really good, thanks for digging into it. Without this patch, clients of the directory iterator need to check for broken symlinks explicitly and issue a call to "i.no_push()". That clearly isn't happening everywhere it should. This patch really improves the API.

Note that after this lands, we'll need to fix up clients of this API in llvm-profdata.cpp and in lldb (inside of FileSpec::EnumerateDirectory).

I have a few more suggestions inline --



================
Comment at: include/llvm/Support/FileSystem.h:959
         *State, path.toStringRef(path_storage), FollowSymlinks);
+    if (!ec)
+      check_current_entry(ec);
----------------
Why not sink the "!ec" test into check_current_entry?


================
Comment at: include/llvm/Support/FileSystem.h:1006
+  // current entry may not exist due to broken symbol links.
+  void check_current_entry(std::error_code &ec) {
+		ErrorOr<basic_file_status> status = State->CurrentEntry.status();
----------------
Could you rename this to something more explicit, such as update_error_code_for_current_entry?


================
Comment at: unittests/Support/Path.cpp:914
+    std::string CurrentFileName = path::filename(i->path());
+    if (std::find(NamesOfBrokenSymlinks.begin(), NamesOfBrokenSymlinks.end(),
+                  CurrentFileName) == NamesOfBrokenSymlinks.end()) {
----------------
You can just write: "find(NamesOfBrokenSymlinks, CurrentFileName)". LLVM defines range-based algorithms in llvm/ADT/STLExtras.h.


================
Comment at: unittests/Support/Path.cpp:918
+    } else {
+      ASSERT_TRUE(ec);
     }
----------------
Should this be something like, ASSERT_EQUAL(ec.category(), std::errc::no_such_file_or_directory)?


================
Comment at: unittests/Support/Path.cpp:925
+	// There should be two "ddd" due to the symbol link from "da" to "dd".
+  expected = {"a", "b", "ba", "bb", "bc", "c", "d", "da", "dd", "ddd", "ddd",
+    					"e"};
----------------
Could you retain the behavior of the original test and continue when a broken symlink is found?

As you did above, you can check that the set of broken symlink names is what you expect.


Repository:
  rL LLVM

https://reviews.llvm.org/D44960