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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits