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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: include/clang/Basic/VirtualFileSystem.h:315
+IntrusiveRefCntPtr<OverlayFileSystem>
+createOverlayOnRealFilesystem(IntrusiveRefCntPtr<FileSystem> TopFS);
+
----------------
ilya-biryukov wrote:
> NIT: I'm not an expert in English, but shouldn't it be createOverlay**Over**Real.....
> Also maybe shorten the suffix: `createOverlayOverRealFS`?
According to [[https://www.thefreedictionary.com/overlay|this dictionary]] overlay can mean "to lay //on// " something. Although `above` could also work to eliminate saying "over" repeatedly.


================
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() {}"));
 
----------------
ilya-biryukov wrote:
> Using `'/'` in paths would probably break on Windows.
> Why do we need to add it in the first place?
Agreed, this will be removed. It was added because overlaying the real-FS changes the working directory in the memory filesystem too, which in the previous patch (where the overlay was done by the ClangTool constructor) broke where the file resides. It's not applicable anymore.

Although it's strange that saying `make check-clang-tooling` did **NOT** execute these tests and said everything passed, I had to run the build `ToolingTest` binary manually!


================
Comment at: unittests/Tooling/ToolingTest.cpp:435
+      newFrontendActionFactory<SyntaxOnlyAction>());
+  EXPECT_NE(0, Tool.run(Action.get()));
+}
----------------
ilya-biryukov wrote:
> 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?)
How can I match on the error message?


https://reviews.llvm.org/D45094