[clang-tools-extra] [clang-tidy] Add detection in container's method except `empty` in `readability-container-size-empty` (PR #154017)

[via cfe-commits]

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