[Lldb-commits] [PATCH] D65677: [VirtualFileSystem] Make the RedirectingFileSystem hold on to its own working directory.

[Sam McCall via Phabricator via lldb-commits]

sammccall added a comment.

Mostly LG, just a couple of possible logic bugs.

Apologies, I was out on vacation and hoped someone else would see this.



================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:650
 
+  bool Fallthrough() const { return ExternalFSValidWD && IsFallthrough; }
+
----------------
this name seems less than ideal:
 - it's very similar to `IsFallthrough` but has different semantics
 - "fallthrough" itself is not a very clear description of this functionality IMO
 - it's spelled wrong per the style guide

I'd suggest `shouldUseExternalFS()` or so


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1057
+    auto EC = ExternalFS->setCurrentWorkingDirectory(Path);
+    ExternalFSValidWD = static_cast<bool>(EC);
+  }
----------------
this seems backwards - error_code converts to true if it's an *error*

add tests?


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1061
+  // Don't change the working directory if the path doesn't exist.
+  if (!exists(Path))
+    return errc::no_such_file_or_directory;
----------------
this seems like it should go at the top? cd to a nonexistent directory should avoid changing state and return an error (which means not marking ExternalFSValidWD as false, I think)


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1065
+  // Non-absolute paths are relative to the current working directory.
+  if (!sys::path::is_absolute(Path)) {
+    SmallString<128> AbsolutePath;
----------------
makeAbsolute already does this check


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65677/new/

https://reviews.llvm.org/D65677