[PATCH] D95092: [VFS] Add a WorkingDirectoryFileSystem

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

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

This looks like the direction I was suggesting; may be in a follow up patch one of us can change `RealFileSystem` to be based on this.

What's missing right now is that the APIs taking paths need to be intercepted, so the paths can be converted to absolute before going to the underlying FS. I'd also prefer if we could put all or most of the guts in a base class that isn't templated, both to reduce code duplication and perhaps sink some of it into the .cpp file.



================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:771
 
+template <typename T> class WorkingDirectoryFileSystem : public T {
+public:
----------------
Please name this `BaseT` to document a little better that it's a base class.


================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:786
+      return llvm::errc::no_such_file_or_directory;
+    WorkingDirectory = Path.str();
+    EC = {};
----------------
I think this should turn `Path` into an absolute path. I'd suggest something like:
```
SmallString<128> CanonicalizedPath;
Path.toVector(CanonicalizedPath);
sys::fs::make_absolute(WorkingDirectory, CanonicalizedPath);
sys::path::remove_dots(CanonicalizedPath);
WorkingDirectory = CanonicalizedPath.str();
```
`remove_dots` is important to avoid `cd ../x` followed by `cd ../y` from endlessly growing the WD into something like `/path/to/../x/../y`.

Also, see below, maybe these guts don't need to be templated and can go in the `.cpp`.


================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:796
+  }
+
+private:
----------------
I think you also need to override every API that takes a (potentially relative) path:
- status
- openFileForRead
- getBufferForFile
- dir_begin
- getRealPath
- isLocal
- makeAbsolute

I'd suggest splitting out `WorkingDirectoryFileSystemBase`, which would not inherit from anything, as a non-templated place to put a couple of helpers:
```
class WorkingDirectoryFileSystemBase {
public:
  // Can be public or protected; doesn't matter since WorkingDirectoryFileSystem
  // inherits privately. Definition should probably go in the .cpp.
  Twine getAdjustedPathTwine(const Twine &Path,
                             SmallVectorImpl<char> &Storage) {
    if (sys::path::is_absolute(Path))
      return Path;
    Path.toVector(Storage);
    sys::fs::make_absolute(WorkingDirectory, Storage);
    return Storage;
  }

  ErrorOr<Status> status(const Twine &Path) {
    SmallString<128> Storage;
    return getWorkingDirectoryUnderlyingFS().status(
        getAdjustedPathTwine(Path, Storage));
  }

  // etc.

protected:
  const FileSystem &getWorkingDirectoryUnderlyingFS() const = 0;
  FileSystem &getWorkingDirectoryUnderlyingFS() {
    return const_cast<FileSystem &>(
        const_cast<const WorkingDirectoryFileSystemBase *>(this)->
            getWorkingDirectoryUnderlyingFS());
  }
private:
  std::string WorkingDirectory;
}
```
and then you're left with:
```
template <class BaseT>
class WorkingDirectoryFileSystem
    : WorkingDirectoryFileSystemBase, public BaseT {
public:
  std::error_code setCurrentWorkingDirectory(const Twine &Path) override {
    return WorkingDirectoryFileSystemBase::setCurrentWorkingDirectory(Path);
  }

  std::error_code makeAbsolute(SmallVectorImpl<char> &Path) override {
    return WorkingDirectoryFileSystemBase::makeAbsolute(Path);
  }

  ErrorOr<Status> status(const Twine &Path) override {
    return WorkingDirectoryFileSystemBase::status(Path);
  }

  // etc.

private:
  const FileSystem &getWorkingDirectoryUnderlyingFS() const final {
    return static_cast<BaseT *>(this);
  }
};
```
or there are other ways to layer it, like passing `static_cast<BaseT *>` down to WorkingDirectoryFileSystemBase as a parameter, or pulling the calls to `getAdjustedPathTwine()` up to the templated class.


================
Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:907-910
+  EC = ErrorFS->setCurrentWorkingDirectory("//root/");
+  ASSERT_TRUE(EC);
+  EC = ErrorFS->setCurrentWorkingDirectory("//root/foo");
+  ASSERT_TRUE(EC);
----------------
You can probably skip assigning `EC` here, make this two one-line statements, unless you're actually going to check what the error is.


================
Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:962
+}
+
 class InMemoryFileSystemTest : public ::testing::Test {
----------------
I think we also to test that the WorkingDirFS will correctly convert relative paths to absolute paths in the other APIs, like `status()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95092