Aligns the interface landed in D41535 to the patch which makes VFS injection more user-friendly. The interface how ClangTidy and the error reporter gets the filesystem to use has also been made more explicit: now ErrorReporter gets the exact Clang Tool('s filesystem layout) that Clang-Tidy used to run checks.
Details
Diff Detail
Event Timeline
clang-tidy/ClangTidy.cpp | ||
---|---|---|
91 | Why do we need to change the signature of ErrorReporter constructor? | |
clang-tidy/ClangTidy.h | ||
229 | ||
clang-tidy/tool/CMakeLists.txt | ||
19 | Why do we need an extra dependency? | |
clang-tidy/tool/ClangTidyMain.cpp | ||
432 | Maybe use a shorter name, e.g. FileSystem? |
clang-tidy/ClangTidy.cpp | ||
---|---|---|
91 | Is it okay interface-wise if the FS used by the ErrorReporter is not the same as the one used by the module that produced the errors? It seems like an undocumented consensus/convention that the same VFS should have been passed here. | |
clang-tidy/tool/CMakeLists.txt | ||
19 | In the current state of the patch, clang-tidy/tool/ClangTidyMain.cpp constructs the ClangTool which constructor requires a std::shared_ptr<PCHContainerOperations>. PCHContainerOperations's definition and implementation is part of the clangFrontend library, and without this dependency, there would be a symbol resolution error. |
clang-tidy/ClangTidy.cpp | ||
---|---|---|
91 | I find actually documenting that the vfs should be the same to be a better option. Or even sharing the FileManager, but that might be more involved. The problem of passing ClangTool is that it's a much more powerful than what we need (e.g. it allows to rerun clang-tidy, which is certainly not something that ErrorReporter should do). | |
clang-tidy/tool/CMakeLists.txt | ||
19 | Thanks for clarification. |
clang-tidy/tool/CMakeLists.txt | ||
---|---|---|
19 | I'm not sure if you need to have both as a dependency just to use ClangTool. If you want to construct an empty PCHContainerOperations to pass as an argument, though, it seems you do. One can wonder where the default argument's (which is a shared pointer to a default constructed object) expression's evaluation is in the object code or if passing nullptr breaks something. You need the dependency because it's your code who wants to run the constructor of the PCH class. |
I'm not sure if passing ClangTool here is an improvement.
Let's ask someone more familiar with clang-tidy. @hokein, @alexfh, WDYT?