diff --git a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp --- a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp @@ -16,6 +16,77 @@ using namespace clang::ast_matchers; namespace clang { +namespace ast_matchers { +AST_POLYMORPHIC_MATCHER_P2(hasAnyArgumentWithParam, + AST_POLYMORPHIC_SUPPORTED_TYPES(CallExpr, + CXXConstructExpr), + internal::Matcher, ArgMatcher, + internal::Matcher, ParamMatcher) { + BoundNodesTreeBuilder Result; + // 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. + BoundNodesTreeBuilder Matches; + unsigned ArgIndex = cxxOperatorCallExpr(callee(cxxMethodDecl())) + .matches(Node, Finder, &Matches) + ? 1 + : 0; + int ParamIndex = 0; + for (; ArgIndex < Node.getNumArgs(); ++ArgIndex) { + BoundNodesTreeBuilder ArgMatches(*Builder); + if (ArgMatcher.matches(*(Node.getArg(ArgIndex)->IgnoreParenCasts()), Finder, + &ArgMatches)) { + BoundNodesTreeBuilder ParamMatches(ArgMatches); + if (expr(anyOf(cxxConstructExpr(hasDeclaration(cxxConstructorDecl( + hasParameter(ParamIndex, ParamMatcher)))), + callExpr(callee(functionDecl( + hasParameter(ParamIndex, ParamMatcher)))))) + .matches(Node, Finder, &ParamMatches)) { + Result.addMatch(ParamMatches); + *Builder = std::move(Result); + return true; + } + } + ++ParamIndex; + } + return false; +} + +AST_MATCHER(Expr, usedInBooleanContext) { + const char *ExprName = "__booleanContextExpr"; + auto Result = + expr(expr().bind(ExprName), + anyOf(hasParent(varDecl(hasType(booleanType()))), + hasParent(cxxConstructorDecl( + hasAnyConstructorInitializer(cxxCtorInitializer( + withInitializer(expr(equalsBoundNode(ExprName))), + forField(hasType(booleanType())))))), + hasParent(fieldDecl(hasType(booleanType()))), + hasParent(stmt(anyOf( + explicitCastExpr(hasDestinationType(booleanType())), + ifStmt(hasCondition(expr(equalsBoundNode(ExprName)))), + doStmt(hasCondition(expr(equalsBoundNode(ExprName)))), + whileStmt(hasCondition(expr(equalsBoundNode(ExprName)))), + forStmt(hasCondition(expr(equalsBoundNode(ExprName)))), + conditionalOperator( + hasCondition(expr(equalsBoundNode(ExprName)))), + parenListExpr(hasParent(varDecl(hasType(booleanType())))), + parenExpr(hasParent( + explicitCastExpr(hasDestinationType(booleanType())))), + returnStmt(forFunction(returns(booleanType()))), + cxxUnresolvedConstructExpr(hasType(booleanType())), + callExpr(hasAnyArgumentWithParam( + expr(equalsBoundNode(ExprName)), + parmVarDecl(hasType(booleanType())))), + binaryOperator(hasAnyOperatorName("&&", "||")), + unaryOperator(hasOperatorName("!")).bind("NegOnSize")))))) + .matches(Node, Finder, Builder); + Builder->removeBindings([ExprName](const BoundNodesMap &Nodes) { + return Nodes.getNode(ExprName).getNodeKind().isNone(); + }); + return Result; +} +} // namespace ast_matchers namespace tidy { namespace readability { @@ -26,18 +97,27 @@ : ClangTidyCheck(Name, Context) {} void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) { - const auto ValidContainer = qualType(hasUnqualifiedDesugaredType( - recordType(hasDeclaration(cxxRecordDecl(isSameOrDerivedFrom( - namedDecl( - has(cxxMethodDecl( - isConst(), parameterCountIs(0), isPublic(), - hasName("size"), - returns(qualType(isInteger(), unless(booleanType())))) - .bind("size")), - has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(), - hasName("empty"), returns(booleanType())) - .bind("empty"))) - .bind("container"))))))); + const auto ValidContainerRecord = cxxRecordDecl(isSameOrDerivedFrom( + namedDecl( + has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(), + hasName("size"), + returns(qualType(isInteger(), unless(booleanType()), + unless(elaboratedType())))) + .bind("size")), + has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(), + hasName("empty"), returns(booleanType())) + .bind("empty"))) + .bind("container"))); + + const auto ValidContainerNonTemplateType = + qualType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(ValidContainerRecord)))); + const auto ValidContainerTemplateType = + qualType(hasUnqualifiedDesugaredType(templateSpecializationType( + hasDeclaration(classTemplateDecl(has(ValidContainerRecord)))))); + + const auto ValidContainer = qualType( + anyOf(ValidContainerNonTemplateType, ValidContainerTemplateType)); const auto WrongUse = traverse( TK_AsIs, @@ -52,18 +132,34 @@ anyOf(hasParent( unaryOperator(hasOperatorName("!")).bind("NegOnSize")), anything()))), - hasParent(explicitCastExpr(hasDestinationType(booleanType()))))); + usedInBooleanContext())); Finder->addMatcher( - cxxMemberCallExpr(on(expr(anyOf(hasType(ValidContainer), + cxxMemberCallExpr(unless(isInTemplateInstantiation()), + on(expr(anyOf(hasType(ValidContainer), hasType(pointsTo(ValidContainer)), - hasType(references(ValidContainer))))), + hasType(references(ValidContainer)))) + .bind("MemberCallObject")), callee(cxxMethodDecl(hasName("size"))), WrongUse, unless(hasAncestor(cxxMethodDecl( ofClass(equalsBoundNode("container")))))) .bind("SizeCallExpr"), this); + Finder->addMatcher( + callExpr(has(cxxDependentScopeMemberExpr( + hasObjectExpression( + expr(anyOf(hasType(ValidContainer), + hasType(pointsTo(ValidContainer)), + hasType(references(ValidContainer)))) + .bind("MemberCallObject")), + hasMemberName("size"))), + WrongUse, + unless(hasAncestor( + cxxMethodDecl(ofClass(equalsBoundNode("container")))))) + .bind("SizeCallExpr"), + this); + // Empty constructor matcher. const auto DefaultConstructor = cxxConstructExpr( hasDeclaration(cxxConstructorDecl(isDefaultConstructor()))); @@ -72,12 +168,11 @@ 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))))); + cxxConstructExpr(hasDeclaration(cxxConstructorDecl(isCopyConstructor())), + has(expr(ignoringImpCasts(DefaultConstructor)))), + cxxConstructExpr(hasDeclaration(cxxConstructorDecl(isMoveConstructor())), + has(expr(ignoringImpCasts(DefaultConstructor)))), + cxxUnresolvedConstructExpr(argumentCountIs(0))); // Match the object being compared. const auto STLArg = anyOf(unaryOperator( @@ -87,6 +182,7 @@ expr(hasType(ValidContainer)).bind("STLObject")); Finder->addMatcher( cxxOperatorCallExpr( + unless(isInTemplateInstantiation()), hasAnyOverloadedOperatorName("==", "!="), anyOf(allOf(hasArgument(0, WrongComparend), hasArgument(1, STLArg)), allOf(hasArgument(0, STLArg), hasArgument(1, WrongComparend))), @@ -94,24 +190,33 @@ cxxMethodDecl(ofClass(equalsBoundNode("container")))))) .bind("BinCmp"), this); + Finder->addMatcher( + binaryOperator(hasAnyOperatorName("==", "!="), + anyOf(allOf(hasLHS(WrongComparend), hasRHS(STLArg)), + allOf(hasLHS(STLArg), hasRHS(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 *MemberCall = Result.Nodes.getNodeAs("SizeCallExpr"); + const auto *MemberCallObject = + Result.Nodes.getNodeAs("MemberCallObject"); const auto *BinCmp = Result.Nodes.getNodeAs("BinCmp"); + const auto *BinCmpTempl = Result.Nodes.getNodeAs("BinCmp"); const auto *BinaryOp = Result.Nodes.getNodeAs("SizeBinaryOp"); const auto *Pointee = Result.Nodes.getNodeAs("Pointee"); const auto *E = - MemberCall - ? MemberCall->getImplicitObjectArgument() + MemberCallObject + ? MemberCallObject : (Pointee ? Pointee : Result.Nodes.getNodeAs("STLObject")); FixItHint Hint; std::string ReplacementText = std::string( 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. + if (IsBinaryOrTernary(E) || isa(E)) { ReplacementText = "(" + ReplacementText + ")"; } if (E->getType()->isPointerType()) @@ -125,7 +230,13 @@ } Hint = FixItHint::CreateReplacement(BinCmp->getSourceRange(), ReplacementText); - } else if (BinaryOp) { // Determine the correct transformation. + } else if (BinCmpTempl) { + if (BinCmpTempl->getOpcode() == BinaryOperatorKind::BO_NE) { + ReplacementText = "!" + ReplacementText; + } + Hint = FixItHint::CreateReplacement(BinCmpTempl->getSourceRange(), + ReplacementText); + } else if (BinaryOp) { // Determine the correct transformation. bool Negation = false; const bool ContainerIsLHS = !llvm::isa(BinaryOp->getLHS()->IgnoreImpCasts()); @@ -195,15 +306,17 @@ "!" + ReplacementText); } - if (MemberCall) { - diag(MemberCall->getBeginLoc(), - "the 'empty' method should be used to check " - "for emptiness instead of 'size'") + auto WarnLoc = MemberCall ? MemberCall->getBeginLoc() : SourceLocation{}; + + if (WarnLoc.isValid()) { + diag(WarnLoc, "the 'empty' method should be used to check " + "for emptiness instead of 'size'") << Hint; } else { - diag(BinCmp->getBeginLoc(), - "the 'empty' method should be used to check " - "for emptiness instead of comparing to an empty object") + WarnLoc = BinCmpTempl ? BinCmpTempl->getBeginLoc() + : (BinCmp ? BinCmp->getBeginLoc() : SourceLocation{}); + diag(WarnLoc, "the 'empty' method should be used to check " + "for emptiness instead of comparing to an empty object") << Hint; } diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp @@ -483,3 +483,192 @@ f(); f(); } + +template +bool neverInstantiatedTemplate() { + std::vector v; + if (v.size()) + ; + // 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()){{$}} + + 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: ; + if (v.size() == 0) + ; + // 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()){{$}} + if (static_cast(v.size())) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:25: 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 (static_cast(!v.empty())){{$}} + if (v.size() && false) + ; + // 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() && false){{$}} + if (!v.size()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:8: 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()){{$}} + + TemplatedContainer templated_container; + if (templated_container.size()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here + // CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}} + if (templated_container != TemplatedContainer()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used + // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here + // CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}} + // CHECK-FIXES-NEXT: ; + while (templated_container.size()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: the 'empty' method should be used + // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here + // CHECK-FIXES: {{^ }}while (!templated_container.empty()){{$}} + + do { + } + while (templated_container.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used + // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here + // CHECK-FIXES: {{^ }}while (!templated_container.empty()); + + for (; templated_container.size();) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: the 'empty' method should be used + // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here + // CHECK-FIXES: {{^ }}for (; !templated_container.empty();){{$}} + + if (true && templated_container.size()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:15: warning: the 'empty' method should be used + // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here + // CHECK-FIXES: {{^ }}if (true && !templated_container.empty()){{$}} + + if (true || templated_container.size()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:15: warning: the 'empty' method should be used + // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here + // CHECK-FIXES: {{^ }}if (true || !templated_container.empty()){{$}} + + if (!templated_container.size()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: the 'empty' method should be used + // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here + // CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}} + + bool b1 = templated_container.size(); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used + // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here + // CHECK-FIXES: {{^ }}bool b1 = !templated_container.empty(); + + bool b2(templated_container.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: the 'empty' method should be used + // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here + // CHECK-FIXES: {{^ }}bool b2(!templated_container.empty()); + + auto b3 = static_cast(templated_container.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: the 'empty' method should be used + // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here + // CHECK-FIXES: {{^ }}auto b3 = static_cast(!templated_container.empty()); + + auto b4 = (bool)templated_container.size(); + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: the 'empty' method should be used + // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here + // CHECK-FIXES: {{^ }}auto b4 = (bool)!templated_container.empty(); + + auto b5 = bool(templated_container.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: the 'empty' method should be used + // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here + // CHECK-FIXES: {{^ }}auto b5 = bool(!templated_container.empty()); + + takesBool(templated_container.size()); + // We don't detect this one because we don't know the parameter of takesBool + // until the type of templated_container.size() is known + + return templated_container.size(); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used + // CHECK-MESSAGES: :44:8: note: method 'TemplatedContainer'::empty() defined here + // CHECK-FIXES: {{^ }}return !templated_container.empty(); +} + +template +void instantiatedTemplateWithSizeCall() { + TypeRequiresSize t; + // The instantiation of the template with std::vector should not + // result in changing the template, because we don't know that + // TypeRequiresSize generally has `.empty()` + if (t.size()) + ; + + if (t == TypeRequiresSize{}) + ; + + if (t != TypeRequiresSize{}) + ; +} + +class TypeWithSize { +public: + TypeWithSize(); + bool operator==(const TypeWithSize &other) const; + bool operator!=(const TypeWithSize &other) const; + + unsigned size() const { return 0; } + // Does not have `.empty()` +}; + +void instantiator() { + instantiatedTemplateWithSizeCall(); + instantiatedTemplateWithSizeCall>(); +} + +class ConstructWithBoolField { + bool B; +public: + ConstructWithBoolField(const std::vector &C) : B(C.size()) {} +// CHECK-MESSAGES: :[[@LINE-1]]:57: warning: the 'empty' method should be used +// CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here +// CHECK-FIXES: {{^ }}ConstructWithBoolField(const std::vector &C) : B(!C.empty()) {} +}; + +struct StructWithNSDMI { + std::vector C; + bool B = C.size(); +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: the 'empty' method should be used +// CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here +// CHECK-FIXES: {{^ }}bool B = !C.empty(); +}; + +int func(const std::vector &C) { + return C.size() ? 0 : 1; +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used +// CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here +// CHECK-FIXES: {{^ }}return !C.empty() ? 0 : 1; +} + +struct Lazy { + constexpr unsigned size() const { return 0; } + constexpr bool empty() const { return true; } +}; + +constexpr Lazy L; +static_assert(!L.size(), ""); +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: the 'empty' method should be used +// CHECK-MESSAGES: :663:18: note: method 'Lazy'::empty() defined here +// CHECK-FIXES: {{^}}static_assert(L.empty(), ""); + +struct StructWithLazyNoexcept { + void func() noexcept(L.size()); +};