diff --git a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp index f0357fb49eff..19307b4cdcbe 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 562cb0d9e99e..b7007a71d94c 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 ` 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 ` 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 3b9e06084183..84bdbd58b85e 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& other) const; bool operator!=(const TemplatedContainer& other) const; - int size() const; + unsigned long size() const; bool empty() const; }; @@ -42,7 +42,7 @@ class PrivateEmpty { public: bool operator==(const PrivateEmpty& other) const; bool operator!=(const PrivateEmpty& 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()) {} + } +}