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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added subscribers: hokein, alexfh.
ilya-biryukov 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()),
----------------
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.


================
Comment at: clang-tidy/ClangTidy.h:229
 void runClangTidy(clang::tidy::ClangTidyContext &Context,
-                  const tooling::CompilationDatabase &Compilations,
-                  ArrayRef<std::string> InputFiles,
-                  llvm::IntrusiveRefCntPtr<vfs::FileSystem> BaseFS,
+                  clang::tooling::ClangTool &Tool,
                   ProfileData *Profile = nullptr);
----------------
I'm not sure if passing `ClangTool` here is an improvement.
Let's ask someone more familiar with clang-tidy. @hokein, @alexfh, WDYT? 


================
Comment at: clang-tidy/tool/CMakeLists.txt:19
   clangBasic
+  clangFrontend
   clangTidy
----------------
Why do we need an extra dependency?


================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:432
   }
-  llvm::IntrusiveRefCntPtr<vfs::FileSystem> BaseFS(
-      VfsOverlay.empty() ? vfs::getRealFileSystem()
+  llvm::IntrusiveRefCntPtr<vfs::FileSystem> VirtualFileSystem(
+      VfsOverlay.empty() ? nullptr
----------------
Maybe use a shorter name, e.g. `FileSystem`?


https://reviews.llvm.org/D45095