[clang-tools-extra] [clang-tidy] Fix false-positives in readability-container-size-empty (PR #74140)
Piotr Zegar via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 1 13:09:30 PST 2023
https://github.com/PiotrZSL created https://github.com/llvm/llvm-project/pull/74140
Added support for size-like method returning signed type, and corrected false positive caused by always-false check for size bellow zero.
Closes #72619
>From 2770caf83fe210cbff94e776c52593920a4cbf8d Mon Sep 17 00:00:00 2001
From: Piotr Zegar <me at piotrzegar.pl>
Date: Fri, 1 Dec 2023 20:59:01 +0000
Subject: [PATCH] [clang-tidy] Fix false-positives in
readability-container-size-empty
Added support for size-like method returning signed type,
and corrected false positive caused by always-false check
for size bellow zero.
---
.../readability/ContainerSizeEmptyCheck.cpp | 32 ++++++++--
clang-tools-extra/docs/ReleaseNotes.rst | 4 +-
.../readability/container-size-empty.cpp | 58 +++++++++++++++++--
3 files changed, 83 insertions(+), 11 deletions(-)
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()) {}
+ }
+}
More information about the cfe-commits
mailing list