aaron.ballman added a comment. In D77572#1981871 <https://reviews.llvm.org/D77572#1981871>, @mgehre wrote:
> Thanks for the comments so far. > I'm a bit lost now. Which changes that cannot wait for the next PR do you > see necessary to get this check merged? I'd be curious to know what @njames93 thinks -- I spot-checked the diagnostics you attached earlier (thank you for those!) and all of them seemed reasonable to me, which suggests there's not an extraordinary amount of false positives. The functionality seems useful in its current state, though as you point out, there are improvements you want to make in follow-up patches. That seems reasonable to me. ================ Comment at: clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp:22-24 +/// Matches a Stmt whose parent is a CompoundStmt, +/// and which is directly followed by +/// a Stmt matching the inner matcher. ---------------- It looks like these comments got formatted a bit strangely -- probably should re-flow them. ================ Comment at: clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp:36 + const auto *I = llvm::find(C->body(), &Node); + assert(I != C->body_end()); // C is parent of Node. + if (++I == C->body_end()) ---------------- I think the comment should turn into a string literal that's part of the assertion. ================ Comment at: clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp:61 + hasBody(allOf(hasDescendant(returns(true)), + unless(anyOf(hasDescendant(breakStmt()), + hasDescendant(returnsButNotTrue)))))) ---------------- Should we reject other ways to break out of the loop, like `goto` or `throw`? ================ Comment at: clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp:98 + + diag(S->getForLoc(), "Replace loop by std%0::any_of() from <algorithm>") + << Ranges; ---------------- clang-tidy diagnostics don't start with a capital letter, and I'd probably drop the `from <algorithm>` part under the assumption the user can figure out the header pretty easily from the diagnostic wording. Also, you should put single quotes around the `std%0::any_of()`. Similar below. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77572/new/ https://reviews.llvm.org/D77572 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
