[clang-tools-extra] [clang-tidy] Fix false-positives in readability-container-size-empty (PR #74140)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-tidy

Author: Piotr Zegar (PiotrZSL)

<details>
<summary>Changes</summary>

Added support for size-like method returning signed type, and corrected false positive caused by always-false check for size bellow zero.

Closes #<!-- -->72619

---
Full diff: https://github.com/llvm/llvm-project/pull/74140.diff


3 Files Affected:

- (modified) clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp (+28-4) 
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+3-1) 
- (modified) clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp (+52-6) 


``````````diff
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
index f0357fb49eff331..19307b4cdcbe3ce 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
@@ -291,10 +291,14 @@ void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
                        OpCode == BinaryOperatorKind::BO_NE))
       return;
 
-    // Always true, no warnings for that.
-    if ((OpCode == BinaryOperatorKind::BO_GE && Value == 0 && ContainerIsLHS) ||
-        (OpCode == BinaryOperatorKind::BO_LE && Value == 0 && !ContainerIsLHS))
-      return;
+    // Always true/false, no warnings for that.
+    if (Value == 0) {
+      if ((OpCode == BinaryOperatorKind::BO_GT && !ContainerIsLHS) ||
+          (OpCode == BinaryOperatorKind::BO_LT && ContainerIsLHS) ||
+          (OpCode == BinaryOperatorKind::BO_LE && !ContainerIsLHS) ||
+          (OpCode == BinaryOperatorKind::BO_GE && ContainerIsLHS))
+        return;
+    }
 
     // Do not warn for size > 1, 1 < size, size <= 1, 1 >= size.
     if (Value == 1) {
@@ -306,12 +310,32 @@ void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
         return;
     }
 
+    // Do not warn for size < 1, 1 > size, size <= 0, 0 >= size for non signed
+    // types
+    if ((OpCode == BinaryOperatorKind::BO_GT && Value == 1 &&
+         !ContainerIsLHS) ||
+        (OpCode == BinaryOperatorKind::BO_LT && Value == 1 && ContainerIsLHS) ||
+        (OpCode == BinaryOperatorKind::BO_GE && Value == 0 &&
+         !ContainerIsLHS) ||
+        (OpCode == BinaryOperatorKind::BO_LE && Value == 0 && ContainerIsLHS)) {
+      const Expr *Container = ContainerIsLHS
+                                  ? BinaryOp->getLHS()->IgnoreImpCasts()
+                                  : BinaryOp->getRHS()->IgnoreImpCasts();
+      if (Container->getType()
+              .getCanonicalType()
+              .getNonReferenceType()
+              ->isSignedIntegerType())
+        return;
+    }
+
     if (OpCode == BinaryOperatorKind::BO_NE && Value == 0)
       Negation = true;
+
     if ((OpCode == BinaryOperatorKind::BO_GT ||
          OpCode == BinaryOperatorKind::BO_GE) &&
         ContainerIsLHS)
       Negation = true;
+
     if ((OpCode == BinaryOperatorKind::BO_LT ||
          OpCode == BinaryOperatorKind::BO_LE) &&
         !ContainerIsLHS)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6d5f49dc0625451..02b57f77132e369 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -402,7 +402,9 @@ Changes in existing checks
 - Improved :doc:`readability-container-size-empty
   <clang-tidy/checks/readability/container-size-empty>` check to
   detect comparison between string and empty string literals and support
-  ``length()`` method as an alternative to ``size()``.
+  ``length()`` method as an alternative to ``size()``. Resolved false positives
+  tied to negative values from size-like methods, and one triggered by size
+  checks below zero.
 
 - Improved :doc:`readability-function-size
   <clang-tidy/checks/readability/function-size>` check configuration to use
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 29ac86cf1b369c2..f010c2c665b1086 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
@@ -33,7 +33,7 @@ class TemplatedContainer {
 public:
   bool operator==(const TemplatedContainer<T>& other) const;
   bool operator!=(const TemplatedContainer<T>& other) const;
-  int size() const;
+  unsigned long size() const;
   bool empty() const;
 };
 
@@ -42,7 +42,7 @@ class PrivateEmpty {
 public:
   bool operator==(const PrivateEmpty<T>& other) const;
   bool operator!=(const PrivateEmpty<T>& other) const;
-  int size() const;
+  unsigned long size() const;
 private:
   bool empty() const;
 };
@@ -61,7 +61,7 @@ struct EnumSize {
 class Container {
 public:
   bool operator==(const Container& other) const;
-  int size() const;
+  unsigned long size() const;
   bool empty() const;
 };
 
@@ -70,13 +70,13 @@ class Derived : public Container {
 
 class Container2 {
 public:
-  int size() const;
+  unsigned long size() const;
   bool empty() const { return size() == 0; }
 };
 
 class Container3 {
 public:
-  int size() const;
+  unsigned long size() const;
   bool empty() const;
 };
 
@@ -85,7 +85,7 @@ bool Container3::empty() const { return this->size() == 0; }
 class Container4 {
 public:
   bool operator==(const Container4& rhs) const;
-  int size() const;
+  unsigned long size() const;
   bool empty() const { return *this == Container4(); }
 };
 
@@ -815,3 +815,49 @@ bool testNotEmptyStringLiterals(const std::string& s)
   using namespace std::string_literals;
   return s == "foo"s;
 }
+
+namespace PR72619 {
+  struct SS {
+    bool empty() const;
+    int size() const;
+  };
+
+  struct SU {
+    bool empty() const;
+    unsigned size() const;
+  };
+
+  void f(const SU& s) {
+    if (s.size() < 0) {}
+    if (0 > s.size()) {}
+    if (s.size() >= 0) {}
+    if (0 <= s.size()) {}
+    if (s.size() < 1)
+      ;
+    // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+    // CHECK-FIXES: {{^    }}if (s.empty()){{$}}
+    if (1 > s.size())
+      ;
+    // CHECK-MESSAGES: :[[@LINE-2]]:13: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+    // CHECK-FIXES: {{^    }}if (s.empty()){{$}}
+    if (s.size() <= 0)
+      ;
+    // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+    // CHECK-FIXES: {{^    }}if (s.empty()){{$}}
+    if (0 >= s.size())
+      ;
+    // CHECK-MESSAGES: :[[@LINE-2]]:14: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+    // CHECK-FIXES: {{^    }}if (s.empty()){{$}}
+  }
+
+  void f(const SS& s) {
+    if (s.size() < 0) {}
+    if (0 > s.size()) {}
+    if (s.size() >= 0) {}
+    if (0 <= s.size()) {}
+    if (s.size() < 1) {}
+    if (1 > s.size()) {}
+    if (s.size() <= 0) {}
+    if (0 >= s.size()) {}
+  }
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/74140