Index: clang-tidy/readability/ContainerSizeEmptyCheck.cpp =================================================================== --- clang-tidy/readability/ContainerSizeEmptyCheck.cpp +++ clang-tidy/readability/ContainerSizeEmptyCheck.cpp @@ -62,23 +62,73 @@ ofClass(equalsBoundNode("container")))))) .bind("SizeCallExpr"), this); + + // Empty constructor matcher. + const auto DefaultConstructor = cxxConstructExpr( + hasDeclaration(cxxConstructorDecl(isDefaultConstructor()))); + // Comparison to empty string or empty constructor. + const auto WrongComparend = anyOf( + ignoringImpCasts(stringLiteral(hasSize(0))), + ignoringImpCasts(cxxBindTemporaryExpr(has(DefaultConstructor))), + ignoringImplicit(DefaultConstructor), + cxxConstructExpr( + hasDeclaration(cxxConstructorDecl(isCopyConstructor())), + has(expr(ignoringImpCasts(DefaultConstructor)))), + cxxConstructExpr( + hasDeclaration(cxxConstructorDecl(isMoveConstructor())), + has(expr(ignoringImpCasts(DefaultConstructor))))); + // Match the object being compared. + const auto STLArg = + anyOf(unaryOperator( + hasOperatorName("*"), + hasUnaryOperand( + expr(hasType(pointsTo(ValidContainer))).bind("Pointee"))), + expr(hasType(ValidContainer)).bind("STLObject")); + const auto STLOuter = anyOf(allOf(ignoringImplicit(declRefExpr().bind("Decl")), + STLArg), + STLArg); + Finder->addMatcher( + cxxOperatorCallExpr( + anyOf(hasOverloadedOperatorName("=="), + hasOverloadedOperatorName("!=")), + anyOf(allOf(hasArgument(0, WrongComparend), hasArgument(1, STLOuter)), + allOf(hasArgument(0, STLOuter), hasArgument(1, WrongComparend))), + unless(hasAncestor( + cxxMethodDecl(ofClass(equalsBoundNode("container")))))) + .bind("BinCmp"), + this); } - void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) { const auto *MemberCall = Result.Nodes.getNodeAs("SizeCallExpr"); + const auto *BinCmp = Result.Nodes.getNodeAs("BinCmp"); const auto *BinaryOp = Result.Nodes.getNodeAs("SizeBinaryOp"); - const auto *E = MemberCall->getImplicitObjectArgument(); + const auto *Pointee = Result.Nodes.getNodeAs("Pointee"); + const auto *Decl = Result.Nodes.getNodeAs("Decl"); + const auto *E = + MemberCall + ? MemberCall->getImplicitObjectArgument() + : (Pointee ? Pointee : Result.Nodes.getNodeAs("STLObject")); FixItHint Hint; std::string ReplacementText = Lexer::getSourceText(CharSourceRange::getTokenRange(E->getSourceRange()), *Result.SourceManager, getLangOpts()); + if (BinCmp && !Decl) { + // Not just a DeclRefExpr, so parenthesize to be on the safe side. + ReplacementText = "(" + ReplacementText + ")"; + } if (E->getType()->isPointerType()) ReplacementText += "->empty()"; else ReplacementText += ".empty()"; - if (BinaryOp) { // Determine the correct transformation. + if (BinCmp) { + if (BinCmp->getOperator() == OO_ExclaimEqual) { + ReplacementText = "!" + ReplacementText; + } + Hint = + FixItHint::CreateReplacement(BinCmp->getSourceRange(), ReplacementText); + } else if (BinaryOp) { // Determine the correct transformation. bool Negation = false; const bool ContainerIsLHS = !llvm::isa(BinaryOp->getLHS()->IgnoreImpCasts()); @@ -148,9 +198,17 @@ "!" + ReplacementText); } - diag(MemberCall->getLocStart(), "the 'empty' method should be used to check " - "for emptiness instead of 'size'") - << Hint; + if (MemberCall) { + diag(MemberCall->getLocStart(), + "the 'empty' method should be used to check " + "for emptiness instead of 'size'") + << Hint; + } else { + diag(BinCmp->getLocStart(), + "the 'empty' method should be used to check " + "for emptiness instead of comparing to an empty object.") + << Hint; + } const auto *Container = Result.Nodes.getNodeAs("container"); const auto *Empty = Result.Nodes.getNodeAs("empty"); Index: test/clang-tidy/readability-container-size-empty.cpp =================================================================== --- test/clang-tidy/readability-container-size-empty.cpp +++ test/clang-tidy/readability-container-size-empty.cpp @@ -3,6 +3,8 @@ namespace std { template struct vector { vector(); + bool operator==(const vector& other) const; + bool operator!=(const vector& other) const; unsigned long size() const; bool empty() const; }; @@ -9,6 +11,11 @@ template struct basic_string { basic_string(); + bool operator==(const basic_string& other) const; + bool operator!=(const basic_string& other) const; + bool operator==(const char *) const; + bool operator!=(const char *) const; + basic_string operator+(const basic_string& other) const; unsigned long size() const; bool empty() const; }; @@ -19,6 +26,8 @@ inline namespace __v2 { template struct set { set(); + bool operator==(const set& other) const; + bool operator!=(const set& other) const; unsigned long size() const; bool empty() const; }; @@ -29,6 +38,8 @@ template class TemplatedContainer { public: + bool operator==(const TemplatedContainer& other) const; + bool operator!=(const TemplatedContainer& other) const; int size() const; bool empty() const; }; @@ -36,6 +47,8 @@ template class PrivateEmpty { public: + bool operator==(const PrivateEmpty& other) const; + bool operator!=(const PrivateEmpty& other) const; int size() const; private: bool empty() const; @@ -54,6 +67,7 @@ class Container { public: + bool operator==(const Container& other) const; int size() const; bool empty() const; }; @@ -75,9 +89,17 @@ bool Container3::empty() const { return this->size() == 0; } +class Container4 { +public: + bool operator==(const Container4& rhs) const; + int size() const; + bool empty() const { return *this == Container4(); } +}; + int main() { std::set intSet; std::string str; + std::string str2; std::wstring wstr; str.size() + 0; str.size() - 0; @@ -87,24 +109,53 @@ ; // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] // CHECK-FIXES: {{^ }}if (intSet.empty()){{$}} - // CHECK-MESSAGES: :23:8: note: method 'set'::empty() defined here + // CHECK-MESSAGES: :32:8: note: method 'set'::empty() defined here + if (intSet == std::set()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness + // CHECK-FIXES: {{^ }}if (intSet.empty()){{$}} + // CHECK-MESSAGES: :32:8: note: method 'set'::empty() defined here if (str.size() == 0) ; // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used // CHECK-FIXES: {{^ }}if (str.empty()){{$}} + if ((str + str2).size() == 0) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if ((str + str2).empty()){{$}} + if (str == "") + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if (str.empty()){{$}} + if (str + str2 == "") + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if ((str + str2).empty()){{$}} if (wstr.size() == 0) ; // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used // CHECK-FIXES: {{^ }}if (wstr.empty()){{$}} + if (wstr == "") + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if (wstr.empty()){{$}} std::vector vect; if (vect.size() == 0) ; // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used // CHECK-FIXES: {{^ }}if (vect.empty()){{$}} + if (vect == std::vector()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if (vect.empty()){{$}} if (vect.size() != 0) ; // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}} + if (vect != std::vector()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}} if (0 == vect.size()) ; // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty' method should be used @@ -113,6 +164,14 @@ ; // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty' method should be used // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}} + if (std::vector() == vect) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if (vect.empty()){{$}} + if (std::vector() != vect) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}} if (vect.size() > 0) ; // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used @@ -172,6 +231,14 @@ ; // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used // CHECK-FIXES: {{^ }}if ((*vect3).empty()){{$}} + if ((*vect3) == std::vector()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if ((vect3)->empty()){{$}} + if (*vect3 == std::vector()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if ((vect3)->empty()){{$}} delete vect3; @@ -180,6 +247,10 @@ ; // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used // CHECK-FIXES: {{^ }}if (vect4.empty()){{$}} + if (vect4 == std::vector()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if (vect4.empty()){{$}} TemplatedContainer templated_container; if (templated_container.size() == 0) @@ -186,18 +257,34 @@ ; // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used // CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}} + if (templated_container == TemplatedContainer()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}} if (templated_container.size() != 0) ; // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used // CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}} + if (templated_container != TemplatedContainer()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}} if (0 == templated_container.size()) ; // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty' method should be used // CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}} + if (TemplatedContainer() == templated_container) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}} if (0 != templated_container.size()) ; // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty' method should be used // CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}} + if (TemplatedContainer() != templated_container) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}} if (templated_container.size() > 0) ; // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used @@ -246,12 +333,20 @@ PrivateEmpty private_empty; if (private_empty.size() == 0) ; + if (private_empty == PrivateEmpty()) + ; if (private_empty.size() != 0) ; + if (private_empty != PrivateEmpty()) + ; if (0 == private_empty.size()) ; + if (PrivateEmpty() == private_empty) + ; if (0 != private_empty.size()) ; + if (PrivateEmpty() != private_empty) + ; if (private_empty.size() > 0) ; if (0 < private_empty.size()) @@ -290,6 +385,10 @@ ; // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used // CHECK-FIXES: {{^ }}if (derived.empty()){{$}} + if (derived == Derived()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if (derived.empty()){{$}} } #define CHECKSIZE(x) if (x.size()) @@ -301,6 +400,10 @@ ; // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty] // CHECK-FIXES: {{^ }}if (!v.empty()){{$}} + if (v == std::vector()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of comparing to an empty object. [readability-container-size-empty] + // CHECK-FIXES: {{^ }}if (v.empty()){{$}} // CHECK-FIXES-NEXT: ; CHECKSIZE(v); // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used @@ -311,6 +414,10 @@ ; // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used // CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}} + if (templated_container != TemplatedContainer()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}} // CHECK-FIXES-NEXT: ; CHECKSIZE(templated_container); // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used