[PATCH] D13430: [VFS] Add an in-memory file system implementation.

[Manuel Klimek via cfe-commits]

klimek added inline comments.

================
Comment at: include/clang/Basic/VirtualFileSystem.h:286-288
@@ +285,5 @@
+  }
+  std::error_code setCurrentWorkingDirectory(const Twine &Path) override {
+    WorkingDirectory = Path.str();
+    return std::error_code();
+  }
----------------
Return errc::success?

================
Comment at: lib/Basic/VirtualFileSystem.cpp:80
@@ -79,1 +79,3 @@
 
+// FIXME: This is copypasted from LLVM's file system implementation.
+std::error_code FileSystem::make_absolute(SmallVectorImpl<char> &path) const {
----------------
What's the proposed fix? :)

================
Comment at: lib/Basic/VirtualFileSystem.cpp:312-316
@@ -229,4 +311,7 @@
 
 void OverlayFileSystem::pushOverlay(IntrusiveRefCntPtr<FileSystem> FS) {
   FSList.push_back(FS);
+  // Synchronize added file systems by duplicating the working directory from
+  // the first one in the list.
+  FS->setCurrentWorkingDirectory(getCurrentWorkingDirectory().get());
 }
----------------
If I read this correctly, it gets and sets the pwd from FS for the first one? That seems superfluous.
(perhaps change the constructor to not use pushOverlay)

================
Comment at: lib/Basic/VirtualFileSystem.cpp:437
@@ +436,3 @@
+  virtual ~InMemoryNode() {}
+  Status &getStatus() { return Stat; }
+  InMemoryNodeKind getKind() const { return Kind; }
----------------
Do we want to make this const?

================
Comment at: lib/Basic/VirtualFileSystem.cpp:439
@@ +438,3 @@
+  InMemoryNodeKind getKind() const { return Kind; }
+  virtual std::string toString(unsigned Indent) = 0;
+};
----------------
This probably wants to be const.

================
Comment at: lib/Basic/VirtualFileSystem.cpp:461
@@ +460,3 @@
+class InMemoryFileAdaptor : public File {
+  InMemoryFile &Parent;
+
----------------
I think parent is a misleading name here (parent implies a hierarchy, but this is a simple adapter). Perhaps "Node".

================
Comment at: lib/Basic/VirtualFileSystem.cpp:476
@@ +475,3 @@
+  std::error_code close() override { return std::error_code(); }
+  void setName(StringRef Name) override { Parent.getStatus().setName(Name); }
+};
----------------
Is this used in the File interface? It seems - strange... is this really a "mv" implementation?

================
Comment at: lib/Basic/VirtualFileSystem.cpp:546-550
@@ +545,7 @@
+        // End of the path, create a new file.
+        Status Stat(Path, "", getNextVirtualUniqueID(),
+                    llvm::sys::TimeValue(ModificationTime), 0, 0,
+                    Buffer->getBufferSize(),
+                    llvm::sys::fs::file_type::regular_file,
+                    llvm::sys::fs::all_all);
+        Dir->addChild(Name, llvm::make_unique<detail::InMemoryFile>(
----------------
I think we'll want to get some of these into the interface. But that's fine in a follow-up patch. Add a FIXME though.

================
Comment at: lib/Basic/VirtualFileSystem.cpp:800
@@ -455,1 +799,3 @@
 
+  std::string WorkingDirectory;
+
----------------
Doesn't the YAML FS work on the same pwd as the current process?

================
Comment at: unittests/Tooling/RewriterTestContext.h:42-45
@@ +41,6 @@
+         DiagnosticPrinter(llvm::outs(), &*DiagOpts),
+         InMemoryFileSystem(new vfs::InMemoryFileSystem),
+         OverlayFileSystem(
+             new vfs::OverlayFileSystem(vfs::getRealFileSystem())),
+         Files(FileSystemOptions(), OverlayFileSystem),
+         Sources(Diagnostics, Files), Rewrite(Sources, Options) {
----------------
Perhaps add a FIXME: To make these tests truly in-memory, we need to overlay the builtin headers.


http://reviews.llvm.org/D13430