Index: clang-tidy/readability/ContainerSizeEmptyCheck.cpp =================================================================== --- clang-tidy/readability/ContainerSizeEmptyCheck.cpp +++ clang-tidy/readability/ContainerSizeEmptyCheck.cpp @@ -7,6 +7,7 @@ // //===----------------------------------------------------------------------===// #include "ContainerSizeEmptyCheck.h" +#include "../utils/ASTUtils.h" #include "../utils/Matchers.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchers.h" @@ -19,6 +20,8 @@ namespace tidy { namespace readability { +using utils::IsBinaryOrTernary; + ContainerSizeEmptyCheck::ContainerSizeEmptyCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context) {} @@ -62,23 +65,70 @@ 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")); + Finder->addMatcher( + cxxOperatorCallExpr( + anyOf(hasOverloadedOperatorName("=="), + hasOverloadedOperatorName("!=")), + anyOf(allOf(hasArgument(0, WrongComparend), hasArgument(1, STLArg)), + allOf(hasArgument(0, STLArg), 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 *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 && IsBinaryOrTernary(E)) { + // 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: clang-tidy/utils/ASTUtils.h =================================================================== --- clang-tidy/utils/ASTUtils.h +++ clang-tidy/utils/ASTUtils.h @@ -18,6 +18,8 @@ // Returns the (closest) Function declaration surrounding |Statement| or NULL. const FunctionDecl *getSurroundingFunction(ASTContext &Context, const Stmt &Statement); +// Determine whether Expr is a Binary or Ternary expression. +bool IsBinaryOrTernary(const Expr *E); } // namespace utils } // namespace tidy } // namespace clang Index: clang-tidy/utils/ASTUtils.cpp =================================================================== --- clang-tidy/utils/ASTUtils.cpp +++ clang-tidy/utils/ASTUtils.cpp @@ -23,6 +23,22 @@ "function", match(stmt(hasAncestor(functionDecl().bind("function"))), Statement, Context)); } + +bool IsBinaryOrTernary(const Expr *E) { + const Expr *E_base = E->IgnoreImpCasts(); + if (clang::isa(E_base) || + clang::isa(E_base)) { + return true; + } + + if (const auto *Operator = + clang::dyn_cast(E_base)) { + return Operator->isInfixBinaryOp(); + } + + return false; +} + } // namespace utils } // namespace tidy } // namespace clang 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,12 +3,19 @@ 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; }; 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,21 @@ 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(); } +}; + +std::string s_func() { + return std::string(); +} + int main() { std::set intSet; std::string str; + std::string str2; std::wstring wstr; str.size() + 0; str.size() - 0; @@ -87,24 +113,57 @@ ; // 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 (s_func() == "") + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-FIXES: {{^ }}if (s_func().empty()){{$}} 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 +172,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 +239,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,24 +255,44 @@ ; // 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) ; // 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 +341,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 +393,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 +408,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 +422,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