aaron.ballman added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:26-28 + // The first argument of an overloaded member operator is the implicit object + // argument of the method which should not be matched against a parameter, so + // we skip over it here. ---------------- Isn't this true for any non-static member function? e.g., should the matcher be looking at `cxxOperatorCallExpr()` at all? ================ Comment at: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:61 + anyOf( + hasParent(explicitCastExpr(hasDestinationType(booleanType()))), + hasParent(ifStmt(hasCondition(expr(equalsBoundNode(exprName))))), ---------------- I take it we can't do `hasParent(anyOf(...))` instead? ================ Comment at: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:67 + hasParent(forStmt(hasCondition(expr(equalsBoundNode(exprName))))), + hasParent(varDecl(hasType(booleanType()))), + hasParent( ---------------- If this matters for var decls, what about field decls in a structure? ================ Comment at: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:69 + hasParent( + parenListExpr(hasParent(varDecl(hasType(booleanType()))))), + hasParent(parenExpr(hasParent( ---------------- Same question here. ================ 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()){{$}} ---------------- 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. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp:631 +}; + +void instantiator() { ---------------- Some other test cases that may be of interest: ``` class C { bool B; public: C(const std::vector<int> &C) : B(C.size()) {} }; struct S { std::vector<int> C; bool B = C.size(); }; int func(const std::vector<int> &C) { return C.size() ? 0 : 1; } struct Lazy { constexpr unsigned size() const { return 0; } constexpr bool empty() const { return true; } }; constexpr Lazy L; static_assert(!L.size(), ""); // This one should get converted struct S { void func() noexcept(L.size()); // This one should not }; ``` 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