Skip to content

Conversation

flovent
Copy link
Contributor

@flovent flovent commented Aug 17, 2025

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

@llvmbot
Copy link
Member

llvmbot commented Aug 17, 2025

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: None (flovent)

Changes

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 --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()) {
+    }
+  }
+};

Copy link
Contributor

@carlosgalvezp carlosgalvezp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@vbvictor vbvictor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vbvictor vbvictor merged commit 1f67b94 into llvm:main Aug 28, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang-tidy] readability-container-size-empty fails to detect calls to member functions
4 participants