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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 17 09:08:14 PST 2018

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?

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list