whisperity added inline comments.

Comment at: include/clang/Basic/VirtualFileSystem.h:315
+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
-      "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?


cfe-commits mailing list

Reply via email to