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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tidy/ClangTidy.cpp:91
 public:
-  ErrorReporter(ClangTidyContext &Context, bool ApplyFixes,
-llvm::IntrusiveRefCntPtr 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`'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

[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/tool/CMakeLists.txt:19
   clangBasic
+  clangFrontend
   clangTidy

ilya-biryukov wrote:
> 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`'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.
I'm not sure if you need to have both as a dependency just to use `ClangTool`. 
If you want to construct an empty `PCHContainerOperations` to pass as an 
argument, though, it seems you do.

One can wonder where the default argument's (which is a shared pointer to a 
default constructed object) expression's evaluation is in the object code or if 
passing `nullptr` breaks something.

You need the dependency because it's **your** code who wants to run the 
constructor of the PCH class.


https://reviews.llvm.org/D45095



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Akin to https://reviews.llvm.org/D45094, pinging this too. 🙂


https://reviews.llvm.org/D45095



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Whisperity via Phabricator via cfe-commits]

whisperity created this revision.
whisperity added a reviewer: alexfh.
whisperity added a project: clang-tools-extra.
Herald added subscribers: dkrupp, rnkovacs, baloghadamsoftware, mgorny.

Aligns the interface landed in https://reviews.llvm.org/D41535 to the patch 
which makes VFS injection more user-friendly. The interface how `ClangTidy` and 
the error reporter gets the filesystem to use has also been made more explicit: 
now `ErrorReporter` gets the exact Clang Tool('s filesystem layout) that 
Clang-Tidy used to run checks.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45095

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidy.h
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/ClangTidyMain.cpp

Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -339,10 +339,8 @@
 
 llvm::IntrusiveRefCntPtr
 getVfsOverlayFromFile(const std::string &OverlayFile) {
-  llvm::IntrusiveRefCntPtr OverlayFS(
-  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
   llvm::ErrorOr> Buffer =
-  OverlayFS->getBufferForFile(OverlayFile);
+  vfs::getRealFileSystem()->getBufferForFile(OverlayFile);
   if (!Buffer) {
 llvm::errs() << "Can't load virtual filesystem overlay file '"
  << OverlayFile << "': " << Buffer.getError().message()
@@ -357,8 +355,7 @@
  << OverlayFile << "'.\n";
 return nullptr;
   }
-  OverlayFS->pushOverlay(FS);
-  return OverlayFS;
+  return FS;
 }
 
 static int clangTidyMain(int argc, const char **argv) {
@@ -432,10 +429,10 @@
 llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
 return 1;
   }
-  llvm::IntrusiveRefCntPtr BaseFS(
-  VfsOverlay.empty() ? vfs::getRealFileSystem()
+  llvm::IntrusiveRefCntPtr VirtualFileSystem(
+  VfsOverlay.empty() ? nullptr
  : getVfsOverlayFromFile(VfsOverlay));
-  if (!BaseFS)
+  if (!VfsOverlay.empty() && !VirtualFileSystem)
 return 1;
 
   ProfileData Profile;
@@ -445,8 +442,10 @@
   llvm::InitializeAllAsmParsers();
 
   ClangTidyContext Context(std::move(OwningOptionsProvider));
-  runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS,
-   EnableCheckProfile ? &Profile : nullptr);
+  ClangTool Tool(OptionsParser.getCompilations(), PathList,
+ std::make_shared(),
+ VirtualFileSystem, true);
+  runClangTidy(Context, Tool, EnableCheckProfile ? &Profile : nullptr);
   ArrayRef Errors = Context.getErrors();
   bool FoundErrors =
   std::find_if(Errors.begin(), Errors.end(), [](const ClangTidyError &E) {
@@ -459,7 +458,7 @@
 
   // -fix-errors implies -fix.
   handleErrors(Context, (FixErrors || Fix) && !DisableFixes, WErrorCount,
-   BaseFS);
+   Tool);
 
   if (!ExportFixes.empty() && !Errors.empty()) {
 std::error_code EC;
Index: clang-tidy/tool/CMakeLists.txt
===
--- clang-tidy/tool/CMakeLists.txt
+++ clang-tidy/tool/CMakeLists.txt
@@ -16,6 +16,7 @@
   clangAST
   clangASTMatchers
   clangBasic
+  clangFrontend
   clangTidy
   clangTidyAndroidModule
   clangTidyAbseilModule
Index: clang-tidy/ClangTidy.h
===
--- clang-tidy/ClangTidy.h
+++ clang-tidy/ClangTidy.h
@@ -16,6 +16,7 @@
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
@@ -225,9 +226,7 @@
 /// \param Profile if provided, it enables check profile collection in
 /// MatchFinder, and will contain the result of the profile.
 void runClangTidy(clang::tidy::ClangTidyContext &Context,
-  const tooling::CompilationDatabase &Compilations,
-  ArrayRef InputFiles,
-  llvm::IntrusiveRefCntPtr BaseFS,
+  clang::tooling::ClangTool &Tool,
   ProfileData *Profile = nullptr);
 
 // FIXME: This interface will need to be significantly extended to be useful.
@@ -238,7 +237,7 @@
 /// clang-format configuration file is found, the given \P FormatStyle is used.
 void handleErrors(ClangTidyContext &Context, bool Fix,
   unsigned &WarningsAsErrorsCount,
-  llvm::IntrusiveRefCntPtr BaseFS);
+  clang::tooling::ClangTool &Tool);
 
 /// \brief Serializes replacements into YAML and writes them to the specified
 /// output stream.
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -38,7 +38,6 @@
 #include "clang/Tooling/DiagnosticsYaml.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/ReplacementsYaml.h"
-#include "clan

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

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 141207.
whisperity added a comment.

Update to be in line with contents in dependency patch.


https://reviews.llvm.org/D45095

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidy.h
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/ClangTidyMain.cpp

Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -339,10 +339,8 @@
 
 llvm::IntrusiveRefCntPtr
 getVfsOverlayFromFile(const std::string &OverlayFile) {
-  llvm::IntrusiveRefCntPtr OverlayFS(
-  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
   llvm::ErrorOr> Buffer =
-  OverlayFS->getBufferForFile(OverlayFile);
+  vfs::getRealFileSystem()->getBufferForFile(OverlayFile);
   if (!Buffer) {
 llvm::errs() << "Can't load virtual filesystem overlay file '"
  << OverlayFile << "': " << Buffer.getError().message()
@@ -357,8 +355,7 @@
  << OverlayFile << "'.\n";
 return nullptr;
   }
-  OverlayFS->pushOverlay(FS);
-  return OverlayFS;
+  return FS;
 }
 
 static int clangTidyMain(int argc, const char **argv) {
@@ -432,10 +429,10 @@
 llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
 return 1;
   }
-  llvm::IntrusiveRefCntPtr BaseFS(
-  VfsOverlay.empty() ? vfs::getRealFileSystem()
+  llvm::IntrusiveRefCntPtr VirtualFileSystem(
+  VfsOverlay.empty() ? nullptr
  : getVfsOverlayFromFile(VfsOverlay));
-  if (!BaseFS)
+  if (!VfsOverlay.empty() && !VirtualFileSystem)
 return 1;
 
   ProfileData Profile;
@@ -445,8 +442,10 @@
   llvm::InitializeAllAsmParsers();
 
   ClangTidyContext Context(std::move(OwningOptionsProvider));
-  runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS,
-   EnableCheckProfile ? &Profile : nullptr);
+  ClangTool Tool(OptionsParser.getCompilations(), PathList,
+ std::make_shared(),
+ vfs::createOverlayOnRealFilesystem(VirtualFileSystem));
+  runClangTidy(Context, Tool, EnableCheckProfile ? &Profile : nullptr);
   ArrayRef Errors = Context.getErrors();
   bool FoundErrors =
   std::find_if(Errors.begin(), Errors.end(), [](const ClangTidyError &E) {
@@ -459,7 +458,7 @@
 
   // -fix-errors implies -fix.
   handleErrors(Context, (FixErrors || Fix) && !DisableFixes, WErrorCount,
-   BaseFS);
+   Tool);
 
   if (!ExportFixes.empty() && !Errors.empty()) {
 std::error_code EC;
Index: clang-tidy/tool/CMakeLists.txt
===
--- clang-tidy/tool/CMakeLists.txt
+++ clang-tidy/tool/CMakeLists.txt
@@ -16,6 +16,7 @@
   clangAST
   clangASTMatchers
   clangBasic
+  clangFrontend
   clangTidy
   clangTidyAndroidModule
   clangTidyAbseilModule
Index: clang-tidy/ClangTidy.h
===
--- clang-tidy/ClangTidy.h
+++ clang-tidy/ClangTidy.h
@@ -16,6 +16,7 @@
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
@@ -225,9 +226,7 @@
 /// \param Profile if provided, it enables check profile collection in
 /// MatchFinder, and will contain the result of the profile.
 void runClangTidy(clang::tidy::ClangTidyContext &Context,
-  const tooling::CompilationDatabase &Compilations,
-  ArrayRef InputFiles,
-  llvm::IntrusiveRefCntPtr BaseFS,
+  clang::tooling::ClangTool &Tool,
   ProfileData *Profile = nullptr);
 
 // FIXME: This interface will need to be significantly extended to be useful.
@@ -238,7 +237,7 @@
 /// clang-format configuration file is found, the given \P FormatStyle is used.
 void handleErrors(ClangTidyContext &Context, bool Fix,
   unsigned &WarningsAsErrorsCount,
-  llvm::IntrusiveRefCntPtr BaseFS);
+  clang::tooling::ClangTool &Tool);
 
 /// \brief Serializes replacements into YAML and writes them to the specified
 /// output stream.
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -38,7 +38,6 @@
 #include "clang/Tooling/DiagnosticsYaml.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/ReplacementsYaml.h"
-#include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Signals.h"
 #include 
@@ -89,9 +88,9 @@
 
 class ErrorReporter {
 public:
-  ErrorReporter(ClangTidyContext &Context, bool ApplyFixes,
-llvm::IntrusiveRefCntPtr BaseFS)
-  : Files(FileSystemOptions(), BaseFS), DiagOpts(new DiagnosticOptions()),
+  ErrorReporter(ClangTidyContext &Context, bool ApplyFixes, Cla

[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 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 InputFiles,
-  llvm::IntrusiveRefCntPtr 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 BaseFS(
-  VfsOverlay.empty() ? vfs::getRealFileSystem()
+  llvm::IntrusiveRefCntPtr VirtualFileSystem(
+  VfsOverlay.empty() ? nullptr

Maybe use a shorter name, e.g. `FileSystem`?


https://reviews.llvm.org/D45095



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 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`'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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits