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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to