[Lldb-commits] [PATCH] D94811: [lldb] Fix fallthrough with strictly virtual working directory.

[Duncan P. N. Exon Smith via Phabricator via lldb-commits]

dexonsmith added a comment.

I'm wondering about this:

> Currently, calling setCurrentWorkingDirectory on RedirectingFileSystem
> will attempt to change the working directory for the external FS and if
> that fails, it will set ExternalFSValidWD to false which prevents
> fallthrough.

I'm worried having inconsistent working directories could be worse than blocking fallthrough. Consider:

  # physical
  /a/b/c/d.c
  /c/README
  
  # virtual
  /x/README

What if someone does `setWorkingDirectory()` for `/a/b/c/d.c` followed by `/x`, and then looks up the relative path `d.c`? Or changes directory to `../c`?

I wonder if there's a way to split up `shouldUseExternalFS()` (or refactor other code) to allow the external FS to be partially used without adding this kind of inconsistency.

- Absolute paths should still work just fine.
- Relative paths can be made absolute by the virtual FS first (if the WD isn't valid), then passed through.



================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:764
 
+  std::error_code setVirtualWorkingDirectory(const Twine &Path);
+
----------------
I think this could use a comment to explain its behaviour since it's not obvious.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1079
 
   // Always change the external FS but ignore its result.
+  if (ExternalFS && SetExternalCWD) {
----------------
This comment looks out-of-date.


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

https://reviews.llvm.org/D94811