[clang-tools-extra] 7f1d757 - [clang-tidy] Fix false-positives in readability-container-size-empty (#74140)
via cfe-commits
cfe-commits at lists.llvm.org
Sun Jan 14 06:49:20 PST 2024
Author: Piotr Zegar
Date: 2024-01-14T15:49:16+01:00
New Revision: 7f1d757fb40f06cc1c6b134d770987b340286996
URL: https://github.com/llvm/llvm-project/commit/7f1d757fb40f06cc1c6b134d770987b340286996
DIFF: https://github.com/llvm/llvm-project/commit/7f1d757fb40f06cc1c6b134d770987b340286996.diff
LOG: [clang-tidy] Fix false-positives in readability-container-size-empty (#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
Added:
Modified:
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
index f0357fb49eff33..19307b4cdcbe3c 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 562cb0d9e99ef0..b7007a71d94c0e 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -467,7 +467,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 3b9e0608418305..84bdbd58b85e96 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