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()), ---------------- whisperity wrote: > 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. 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). ================ Comment at: clang-tidy/tool/CMakeLists.txt:19 clangBasic + clangFrontend clangTidy ---------------- whisperity wrote: > 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. Thanks for clarification. Does it mean that to use `ClangTool` one needs both a dependency on `clangTooling` and `clangFrontend`? That's weird, given that `clangTooling` itself depends on `clangFrontend` and it would be nice if the buildsystem actually propagated those. https://reviews.llvm.org/D45095 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
