llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: None (flovent) <details> <summary>Changes</summary> `readability-container-size-empty` skip check in container's all method, but only `empty` method is nesscessary to skip because it's usually implemented by `size`, so this patch allows reporting in other methods. Note: testcase about not reporting in `empty` is already added in the past, so i only add testcases for other methods. Closes #<!-- -->152649 --- Full diff: https://github.com/llvm/llvm-project/pull/154017.diff 3 Files Affected: - (modified) clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp (+45-22) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+2-1) - (modified) clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp (+43) ``````````diff diff --git a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp index c4dc319f23c38..38c0330c02a7e 100644 --- a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp @@ -110,6 +110,25 @@ AST_MATCHER_P(UserDefinedLiteral, hasLiteral, return false; } +AST_MATCHER_P(CXXMethodDecl, hasCanonicalDecl, + clang::ast_matchers::internal::Matcher<CXXMethodDecl>, + InnerMatcher) { + return InnerMatcher.matches(*Node.getCanonicalDecl(), Finder, Builder); +} + +AST_POLYMORPHIC_MATCHER_P( + matchMemberName, + AST_POLYMORPHIC_SUPPORTED_TYPES(MemberExpr, CXXDependentScopeMemberExpr), + std::string, MemberName) { + if (const auto *E = dyn_cast<MemberExpr>(&Node)) + return E->getMemberDecl()->getName() == MemberName; + + if (const auto *E = dyn_cast<CXXDependentScopeMemberExpr>(&Node)) + return E->getMember().getAsString() == MemberName; + + return false; +} + } // namespace using utils::isBinaryOrTernary; @@ -140,9 +159,10 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) { const auto ValidContainerNonTemplateType = qualType(hasUnqualifiedDesugaredType( recordType(hasDeclaration(ValidContainerRecord)))); - const auto ValidContainerTemplateType = - qualType(hasUnqualifiedDesugaredType(templateSpecializationType( - hasDeclaration(classTemplateDecl(has(ValidContainerRecord)))))); + const auto ValidContainerTemplateType = qualType(hasUnqualifiedDesugaredType( + anyOf(templateSpecializationType( + hasDeclaration(classTemplateDecl(has(ValidContainerRecord)))), + injectedClassNameType(hasDeclaration(ValidContainerRecord))))); const auto ValidContainer = qualType( anyOf(ValidContainerNonTemplateType, ValidContainerTemplateType)); @@ -155,6 +175,9 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) { .bind("SizeBinaryOp")), usedInBooleanContext()); + const auto NotInEmptyMethodOfContainer = unless( + forCallable(cxxMethodDecl(hasCanonicalDecl(equalsBoundNode("empty"))))); + Finder->addMatcher( cxxMemberCallExpr( argumentCountIs(0), @@ -164,25 +187,23 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) { .bind("MemberCallObject")), callee( cxxMethodDecl(hasAnyName("size", "length")).bind("SizeMethod")), - WrongUse, - unless(hasAncestor( - cxxMethodDecl(ofClass(equalsBoundNode("container")))))) + WrongUse, NotInEmptyMethodOfContainer) .bind("SizeCallExpr"), this); Finder->addMatcher( - callExpr(argumentCountIs(0), - has(cxxDependentScopeMemberExpr( - hasObjectExpression( - expr(anyOf(hasType(ValidContainer), - hasType(pointsTo(ValidContainer)), - hasType(references(ValidContainer)))) - .bind("MemberCallObject")), - anyOf(hasMemberName("size"), hasMemberName("length"))) - .bind("DependentExpr")), - WrongUse, - unless(hasAncestor( - cxxMethodDecl(ofClass(equalsBoundNode("container")))))) + callExpr( + argumentCountIs(0), + has(mapAnyOf(memberExpr, cxxDependentScopeMemberExpr) + .with( + hasObjectExpression( + expr(anyOf(hasType(ValidContainer), + hasType(pointsTo(ValidContainer)), + hasType(references(ValidContainer)))) + .bind("MemberCallObject")), + anyOf(matchMemberName("size"), matchMemberName("length"))) + .bind("MemberExpr")), + WrongUse, NotInEmptyMethodOfContainer) .bind("SizeCallExpr"), this); @@ -217,8 +238,7 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) { hasAnyOperatorName("==", "!="), hasOperands(WrongComparend, STLArg), unless(allOf(hasLHS(hasType(ExcludedComparisonTypesMatcher)), hasRHS(hasType(SameExcludedComparisonTypesMatcher)))), - unless(hasAncestor( - cxxMethodDecl(ofClass(equalsBoundNode("container")))))) + NotInEmptyMethodOfContainer) .bind("BinCmp"), this); } @@ -382,9 +402,12 @@ void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) { Diag << SizeMethod; else if (const auto *DependentExpr = Result.Nodes.getNodeAs<CXXDependentScopeMemberExpr>( - "DependentExpr")) + "MemberExpr")) Diag << DependentExpr->getMember(); - else + else if (const auto *ME = + Result.Nodes.getNodeAs<MemberExpr>("MemberExpr")) { + Diag << ME->getMemberNameInfo().getName(); + } else Diag << "unknown method"; Diag << Hint; } else { diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index fd81b0d47e82b..a0ef1b6767cbe 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -202,7 +202,8 @@ Changes in existing checks - Improved :doc:`readability-container-size-empty <clang-tidy/checks/readability/container-size-empty>` check by correctly - generating fix-it hints when size method is called from implicit ``this``. + generating fix-it hints when size method is called from implicit ``this`` + and adding detection in container's method except ``empty``. - Improved :doc:`readability-identifier-naming <clang-tidy/checks/readability/identifier-naming>` check by ignoring 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 index b1e68672a3a9a..decc9d2782218 100644 --- 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 @@ -908,3 +908,46 @@ class foo : public std::string{ }; } + +class ReportInContainerNonEmptyMethod { +public: + int size() const; + bool empty() const; + + void doit() { + if (!size()) { + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used to check for emptiness instead of 'size' + // CHECK-FIXES: if (empty()) + } + } +}; + +template <typename T> +class ReportInTemplateContainerNonEmptyMethod { +public: + int size() const; + bool empty() const; + + void doit() { + if (!size()) { + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used to check for emptiness instead of 'size' + // CHECK-FIXES: if (empty()) { + } + } +}; + + + +class ReportInContainerNonEmptyMethodCompare { +public: + bool operator==(const ReportInContainerNonEmptyMethodCompare& other) const; + int size() const; + bool empty() const; + + void doit() { + if (*this == ReportInContainerNonEmptyMethodCompare()) { + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: the 'empty' method should be used to check for emptiness instead of comparing to an empty object + // CHECK-FIXES: if (this->empty()) { + } + } +}; `````````` </details> https://github.com/llvm/llvm-project/pull/154017 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
