aaron.ballman added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:56 +AST_MATCHER(Expr, usedInBooleanContext) { + std::string exprName = "__booleanContextExpr"; + auto result = ---------------- ================ Comment at: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:57 + std::string exprName = "__booleanContextExpr"; + auto result = + expr(expr().bind(exprName), ---------------- ================ Comment at: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:84 + .matches(Node, Finder, Builder); + Builder->removeBindings([this, exprName](const BoundNodesMap &Nodes) { + return Nodes.getNode(exprName).getNodeKind().isNone(); ---------------- Might as well drop the `this` from here. ================ Comment at: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:67 + hasParent(forStmt(hasCondition(expr(equalsBoundNode(exprName))))), + hasParent(varDecl(hasType(booleanType()))), + hasParent( ---------------- steveire wrote: > aaron.ballman wrote: > > If this matters for var decls, what about field decls in a structure? > Not sure. What would this look like in a test? I was thinking this one: ``` struct S { std::vector<int> C; bool B = C.size(); }; ``` but that seems to be handled properly already. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp:493 + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] + // CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here + // CHECK-FIXES: {{^ }}if (!v.empty()){{$}} ---------------- steveire wrote: > aaron.ballman wrote: > > The quoting here is unfortunate. It's not part of your patch, but it could > > be solved by calling `Container->getName()` and manually quoting the note > > text if you wanted to hit it in a follow-up (I'd consider that an NFC > > change, no need to review). You could also address it as part of this patch > > if you felt like it. > That will be a follow-up. SGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91302/new/ https://reviews.llvm.org/D91302 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits