[PATCH] D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: include/clang/Basic/VirtualFileSystem.h:313
+/// \brief Creates a \p vfs::OverlayFileSystem which overlays the given file
+/// system above the 'real' file system, as seen by the operating system.
+IntrusiveRefCntPtr<OverlayFileSystem>
----------------
I suggest leaving out the quotes around 'real'


================
Comment at: lib/Basic/VirtualFileSystem.cpp:328
 void OverlayFileSystem::pushOverlay(IntrusiveRefCntPtr<FileSystem> FS) {
+  // FIXME: OverlayFS containing another one in its stack could be flattened.
   FSList.push_back(FS);
----------------
I generally agree that it might be useful, but given that we can't use `dynamic_cast` in LLVM code addressing this `FIXME` is probably not worth the effort.

And this patch is probably not the right place to add this comment, since it doesn't change `OverlayFileSystem` in any way.


================
Comment at: unittests/Tooling/ToolingTest.cpp:411
   InMemoryFileSystem->addFile(
-      "a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
+      "/a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
 
----------------
Using `'/'` in paths would probably break on Windows.
Why do we need to add it in the first place?


================
Comment at: unittests/Tooling/ToolingTest.cpp:435
+      newFrontendActionFactory<SyntaxOnlyAction>());
+  EXPECT_NE(0, Tool.run(Action.get()));
+}
----------------
It's not clear what this tests expects when looking at the code
A comment would be helpful.

Also consider matching on an actual error diagnostic (i.e. "<cstdio> not found", right?)


https://reviews.llvm.org/D45094