These changes introduce support for -vfsoverlay option in Clang-Tidy
The main reason for it:
In IDE integration we have two different pieces of source: editor and physical file.
File is placed in some project directory (let's call it "A")
We want to run clang-tidy exactly in this project directory "A", in order to get all .clang-tidy files in parent directories along with correct resolution for all relative include paths.
But in IDE, saving mechanism perfoms periodically.
So, to perform analysis of actual text on the fly, we need to save document on every modification (which is inefficient) or generate file in the temporary directory with actual text (it also leads to skipping .clang-tidy files and complicates include search paths resolution)
-vfsoverlay approach allows us to analyze source file in the project directory "A" but with actual data placed in another directory.
Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
Event Timeline
IIUC, you want to pass an overlay so that clang-tidy will read the file opened in the IDE from a different directory?
Could you add -ivfsoverlay option to your compile command instead?
$ clang --help | grep vfs -ivfsoverlay <value> Overlay the virtual filesystem described by file over the real file system
@ilya-biryukov
Yes, this is exactly what i need.
Unfortunately, -ivfsoverlay in the compile commands works for the compiler invocation, but it doesn't work for tooling.
E.g. this call:
clang-tidy -checks=* <file> -- -ivfsoverlay=<yaml_file>
has no effect.
FYI, i've create revision to support ivfsoverlay option in Tooling: https://reviews.llvm.org/D41594
This looks like a bug in tooling, but let's wait for responses on the other therad.
It seems that clang-tidy will terribly broken with per-translation-unit overlays anyway. The problem is that clang-tidy seems to report errors and fixits in ErrorReporter class after running the tooling invocation, therefore it won't see any overlays that were local to each translation unit and may report wrong ranges, etc.
Probably global overlays (i.e. this patch) is probably the way to go.
clang-tidy/ClangTidy.cpp | ||
---|---|---|
528 | Could we add a defaulted vfs::FileSystem BaseFS = getRealFileSystem() parameter to a constructor of ClangTool instead? |
Second part here (provided default virtual filesystem argument to ClangTool constructor): https://reviews.llvm.org/D41947
Sorry for the delay
clang-tidy/ClangTidy.cpp | ||
---|---|---|
92–94 | We should pass BaseFS here as a parameter, similar to how we do that in ClangTool. | |
93 | DiagnosticsEngine& seems to be used to report errors in the source code, while the things we're seeing here are errors when initializing clang-tidy. | |
527 | We should construct vfs before passing it to ClangTool and not modify it later. | |
clang-tidy/ClangTidyOptions.h | ||
114 ↗ | (On Diff #129440) | I think we should avoid putting VfsOverlay into ClangTidyOptions. Is there any reason why having a flag in ClangTidyMain.cpp is not enough? |
clang-tidy/tool/ClangTidyMain.cpp | ||
---|---|---|
345 | This code will print only the enum's integral value, we want to print an actual error message. | |
441 | We should only create an overlay is -vfsoverlay was passed and use getRealFileSystem without wrappers in the common case. | |
444 | Could we stop clang-tidy with an error if we couldn't create an overlay? That seems like a better option than silently running without an overlay when it was actually specified. |
Just a few more NITs and we're good to go
clang-tidy/tool/ClangTidyMain.cpp | ||
---|---|---|
348 | We should probably print the error message (in addition to "not found", we could have permission errors, etc.) | |
354 | NIT: if (!FS) is equivalent, but less typing | |
439 | NIT: remove braces around a single-statement return. |
This looks good, but we should add a test.
Should've noticed that before, sorry for the slowing this down a bit more. After the test is there, I'm happy to commit this change for you.
IIUC, it will be little bit difficult to test it, because whole logic placed in the ClangTidyMain.
All existing clang-tidy unit tests use direct calls of ToolInvocation which is doesn't know about vfsoverlay options.
We could test it with a lit test that runs clang-tidy directly and checks output using FileCheck. See clang-tools-extra/test/clang-tidy for clang-tidy tests using lit and clang/test/VFS for lit tests of vfs overlays in clang.
DiagnosticsEngine& seems to be used to report errors in the source code, while the things we're seeing here are errors when initializing clang-tidy.
Could we move the code that creates vfs into ClangTidyMain.cpp and report errors directly into llvm::errs, similar to how we do that for other flags like list-checks, etc.