[PATCH] D54204: [clangd] Initial clang-tidy diagnostics support.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Looks mostly good, just a few nits.

This patch contains two parts (clang-tidy and clangd), I think we could split into two,  but I'm not insisting, up to you.



================
Comment at: clang-tidy/modernize/LoopConvertUtils.h:59
 
   /// \brief Run the analysis on the TranslationUnitDecl.
   ///
----------------
The comment is out-of-date.


================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:518
 void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(translationUnitDecl().bind("top"), this);
+  Finder->addMatcher(matchOnce(&MatchedOnce), this);
 
----------------
maybe add a comment  describing we are trying to find the top level decl?


================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:566
     replaceCompoundReturnWithCondition(Result, Compound, true);
-  else if (const auto TU = Result.Nodes.getNodeAs<Decl>("top"))
-    Visitor(this, Result).TraverseDecl(const_cast<Decl*>(TU));
+  else // MatchOnce matcher
+    Visitor(this, Result).TraverseAST(*Result.Context);
----------------
add an `assert (MatchOnce)`?


================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:82
 
+  bool MatchedOnce = false;
   const bool ChainedConditionalReturn;
----------------
The name seems a bit unclear for readers without any context, maybe `FoundTopDecl`?


================
Comment at: clangd/ClangdUnit.cpp:158
+  // Clang-tidy has some limitiations to ensure reasonable performance:
+  //  - checks don't see all preprocessor events in the preamble
+  //  - matchers run only over the main-file top-level decls (and can't see
----------------
just want to make sure -- do we receive all preprocessor events in the main file when we use preamble? We have a few checks that generate `#include` insertion as part of FixIt.

`TestTU` doesn't use preamble, it would be nice to have a test running on a main file with a real preamble, but this can be a follow-up I think.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54204