[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:472
   if (Info.hasSourceManager())
     checkFilters(Info.getLocation(), Info.getSourceManager());
 }
----------------
nik wrote:
> gribozavr wrote:
> > It seems like the `checkFilters` call should not be skipped even if we have another diagnostic engine.
> checkFilters() sets some state so that later finalizeLastError() can remove diagnostics/errors from ClangTidyDiagnosticConsumer::Errors and also track some statistics.
> 
> Statistics are not relevant for the pluginc case as they should not be printed out for that case.
> The filtering happening in finalizeLastError() does not look relevant as the plugin currently only supports the "-checks=..." argument but not the e.g. header and line filter. When checks are created in ClangTidyCheckFactories::createChecks, the "-checks=..." argument is honored, so that the !Context.isCheckEnabled(...) in finalizeLastError() is also not relevant.
> 
> I agree that checkFilters()/finalizeLastError() will be needed once the plugin case should support the other filtering options (header + line filter), but note that this goes beyond the scope of this change here, which is NOLINT filtering.
> 
> This is all new to me, so if I miss something regarding the checkFilters()/finalizeLastError() thingy, please tell me, preferably with an example if possible :)
It might be not relevant right now, but that code has invariants about how those two functions are called. Even if things happen to work, I think this code is too complicated to allow breaking those invariants in some cases.


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:207
   CheckNamesByDiagnosticID.try_emplace(ID, CheckName);
+  CustomDiagIdParamsByDiagnosticID.try_emplace(
+      ID, CustomDiagIdParams(Level, FormatString));
----------------
Did you consider adding this query API to DiagnosticIDs instead? To me it looks weird that creation of custom diag IDs is handled by one class (DiagnosticIDs), and queries -- by another (ClangTidyContext).


================
Comment at: test/clang-tidy/nolintnextline-plugin.cpp:47
+
+// RUN: c-index-test -test-load-source-reparse 2 all %s -Xclang -add-plugin -Xclang clang-tidy -Xclang -plugin-arg-clang-tidy -Xclang -checks='-*,google-explicit-constructor' 2>&1 | FileCheck %s
----------------
Why not on the first line?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487