[PATCH] D41535: Add -vfsoverlay option for Clang-Tidy

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.

Sorry for the delay



================
Comment at: clang-tidy/ClangTidy.cpp:93
+                            vfs::OverlayFileSystem &OverlayFS) {
+  if (OverlayFile.empty())
+    return;
----------------
`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.


================
Comment at: clang-tidy/ClangTidy.cpp:115
   ErrorReporter(ClangTidyContext &Context, bool ApplyFixes)
-      : Files(FileSystemOptions()), DiagOpts(new DiagnosticOptions()),
+      : OverlayFS(new vfs::OverlayFileSystem(vfs::getRealFileSystem())),
+        Files(FileSystemOptions(), OverlayFS), 
----------------
We should pass `BaseFS` here as a parameter, similar to how we do that in `ClangTool`.
Piping it here from `clangdTidyMain` seems trivial.


================
Comment at: clang-tidy/ClangTidy.cpp:556
 
+  if (Context.getOptions().VfsOverlay) {
+    pushVfsOverlayFromFile(*Context.getOptions().VfsOverlay,
----------------
We should construct vfs before passing it to `ClangTool` and not modify it later.
I suggest creating `BaseFS` in `clangTidyMain`, see other comments too.


================
Comment at: clang-tidy/ClangTidyOptions.h:114
+  ///        over the real file system
+  llvm::Optional<std::string> VfsOverlay;
 };
----------------
I think we should avoid putting `VfsOverlay` into `ClangTidyOptions`. Is there any reason why having a flag in `ClangTidyMain.cpp` is not enough?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41535