[PATCH] D45095: [clang-tidy] Align usage of ClangTool interface with new VFS injection

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: clang-tidy/ClangTidy.cpp:91
 public:
-  ErrorReporter(ClangTidyContext &Context, bool ApplyFixes,
-                llvm::IntrusiveRefCntPtr<vfs::FileSystem> BaseFS)
-      : Files(FileSystemOptions(), BaseFS), DiagOpts(new DiagnosticOptions()),
+  ErrorReporter(ClangTidyContext &Context, bool ApplyFixes, ClangTool &Tool)
+      : Files(FileSystemOptions(), Tool.getFiles().getVirtualFileSystem()),
----------------
ilya-biryukov wrote:
> Why do we need to change the signature of `ErrorReporter` constructor?
> Broadening the accepted interface does not seem right. It only needs the vfs and the clients could get vfs from `ClangTool` themselves.
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.


================
Comment at: clang-tidy/tool/CMakeLists.txt:19
   clangBasic
+  clangFrontend
   clangTidy
----------------
ilya-biryukov wrote:
> Why do we need an extra dependency?
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.


https://reviews.llvm.org/D45095