Skip to content

Commit

Permalink
[clang-tidy] readability-container-size-empty handle std::string leng…
Browse files Browse the repository at this point in the history 
…th()

Extends readability-container-size-empty to check std::string length() similar to size().

Fixes: #37603

Co-authored-by: Dmitry Venikov <[email protected]>

Reviewed By: carlosgalvezp

Differential Revision: https://reviews.llvm.org/D56644
  • Loading branch information
@PiotrZSL
PiotrZSL committed Aug 31, 2023
1 parent 8942d30 commit efebb4e
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 43 deletions.
61 changes: 37 additions & 24 deletions clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,14 @@ void ContainerSizeEmptyCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {

void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
const auto ValidContainerRecord = cxxRecordDecl(isSameOrDerivedFrom(
namedDecl(
has(cxxMethodDecl(
isConst(), parameterCountIs(0), isPublic(), hasName("size"),
returns(qualType(isIntegralType(), unless(booleanType()))))
.bind("size")),
has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(),
hasName("empty"), returns(booleanType()))
.bind("empty")))
namedDecl(has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(),
hasAnyName("size", "length"),
returns(qualType(isIntegralType(),
unless(booleanType()))))
.bind("size")),
has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(),
hasName("empty"), returns(booleanType()))
.bind("empty")))
.bind("container")));

const auto ValidContainerNonTemplateType =
Expand All @@ -149,24 +149,28 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
usedInBooleanContext());

Finder->addMatcher(
cxxMemberCallExpr(on(expr(anyOf(hasType(ValidContainer),
hasType(pointsTo(ValidContainer)),
hasType(references(ValidContainer))))
.bind("MemberCallObject")),
callee(cxxMethodDecl(hasName("size"))), WrongUse,
unless(hasAncestor(cxxMethodDecl(
ofClass(equalsBoundNode("container"))))))
cxxMemberCallExpr(
on(expr(anyOf(hasType(ValidContainer),
hasType(pointsTo(ValidContainer)),
hasType(references(ValidContainer))))
.bind("MemberCallObject")),
callee(
cxxMethodDecl(hasAnyName("size", "length")).bind("SizeMethod")),
WrongUse,
unless(hasAncestor(
cxxMethodDecl(ofClass(equalsBoundNode("container"))))))
.bind("SizeCallExpr"),
this);

Finder->addMatcher(
callExpr(has(cxxDependentScopeMemberExpr(
hasObjectExpression(
expr(anyOf(hasType(ValidContainer),
hasType(pointsTo(ValidContainer)),
hasType(references(ValidContainer))))
.bind("MemberCallObject")),
hasMemberName("size"))),
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"))))))
Expand Down Expand Up @@ -333,9 +337,18 @@ void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
auto WarnLoc = MemberCall ? MemberCall->getBeginLoc() : SourceLocation{};

if (WarnLoc.isValid()) {
diag(WarnLoc, "the 'empty' method should be used to check "
"for emptiness instead of 'size'")
<< Hint;
auto Diag = diag(WarnLoc, "the 'empty' method should be used to check "
"for emptiness instead of %0");
if (const auto *SizeMethod =
Result.Nodes.getNodeAs<NamedDecl>("SizeMethod"))
Diag << SizeMethod;
else if (const auto *DependentExpr =
Result.Nodes.getNodeAs<CXXDependentScopeMemberExpr>(
"DependentExpr"))
Diag << DependentExpr->getMember();
else
Diag << "unknown method";
Diag << Hint;
} else {
WarnLoc = BinCmpTempl
? BinCmpTempl->getBeginLoc()
Expand Down
4 changes: 2 additions & 2 deletions clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@

namespace clang::tidy::readability {

/// Checks whether a call to the `size()` method can be replaced with a call to
/// `empty()`.
/// Checks whether a call to the `size()`/`length()` method can be replaced with
/// a call to `empty()`.
///
/// The emptiness of a container should be checked using the `empty()` method
/// instead of the `size()` method. It is not guaranteed that `size()` is a
Expand Down
3 changes: 2 additions & 1 deletion clang-tools-extra/docs/ReleaseNotes.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,8 @@ 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.
detect comparison between string and empty string literals and support
``length()`` method as an alternative to ``size()``.

- Improved :doc:`readability-identifier-naming
<clang-tidy/checks/readability/identifier-naming>` check to emit proper
Expand Down
21 changes: 11 additions & 10 deletions clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,24 @@ readability-container-size-empty
================================


Checks whether a call to the ``size()`` method can be replaced with a call to
``empty()``.
Checks whether a call to the ``size()``/``length()`` method can be replaced
with a call to ``empty()``.

The emptiness of a container should be checked using the ``empty()`` method
instead of the ``size()`` method. It is not guaranteed that ``size()`` is a
constant-time function, and it is generally more efficient and also shows
clearer intent to use ``empty()``. Furthermore some containers may implement
the ``empty()`` method but not implement the ``size()`` method. Using
``empty()`` whenever possible makes it easier to switch to another container in
the future.
instead of the ``size()``/``length()`` method. It is not guaranteed that
``size()``/``length()`` is a constant-time function, and it is generally more
efficient and also shows clearer intent to use ``empty()``. Furthermore some
containers may implement the ``empty()`` method but not implement the ``size()``
or ``length()`` method. Using ``empty()`` whenever possible makes it easier to
switch to another container in the future.

The check issues warning if a container has ``size()`` and ``empty()`` methods
matching following signatures:
The check issues warning if a container has ``empty()`` and ``size()`` or
``length()`` methods matching following signatures:

.. code-block:: c++

size_type size() const;
size_type length() const;
bool empty() const;

`size_type` can be any kind of integer type.
Expand Down
1 change: 1 addition & 0 deletions clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ struct basic_string {

bool empty() const;
size_type size() const;
size_type length() const;

_Type& append(const C *s);
_Type& append(const C *s, size_type n);
Expand Down
39 changes: 33 additions & 6 deletions clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,13 @@ bool returnsBool() {
std::string str2;
std::wstring wstr;
(void)(str.size() + 0);
(void)(str.length() + 0);
(void)(str.size() - 0);
(void)(str.length() - 0);
(void)(0 + str.size());
(void)(0 + str.length());
(void)(0 - str.size());
(void)(0 - str.length());
if (intSet.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
Expand All @@ -128,11 +132,19 @@ bool returnsBool() {
// CHECK-FIXES: {{^ }}if (s_func().empty()){{$}}
if (str.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
// CHECK-FIXES: {{^ }}if (str.empty()){{$}}
if (str.length() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
// CHECK-FIXES: {{^ }}if (str.empty()){{$}}
if ((str + str2).size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
// CHECK-FIXES: {{^ }}if ((str + str2).empty()){{$}}
if ((str + str2).length() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
// CHECK-FIXES: {{^ }}if ((str + str2).empty()){{$}}
if (str == "")
;
Expand All @@ -144,7 +156,11 @@ bool returnsBool() {
// CHECK-FIXES: {{^ }}if ((str + str2).empty()){{$}}
if (wstr.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
// CHECK-FIXES: {{^ }}if (wstr.empty()){{$}}
if (wstr.length() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length'
// CHECK-FIXES: {{^ }}if (wstr.empty()){{$}}
if (wstr == L"")
;
Expand All @@ -153,7 +169,7 @@ bool returnsBool() {
std::vector<int> vect;
if (vect.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size'
// CHECK-FIXES: {{^ }}if (vect.empty()){{$}}
if (vect == std::vector<int>())
;
Expand Down Expand Up @@ -508,6 +524,17 @@ template <typename T> void f() {
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
// CHECK-MESSAGES: :37:8: note: method 'TemplatedContainer'::empty() defined here
// CHECK-FIXES: CHECKSIZE(templated_container);
std::basic_string<T> s;
if (s.size())
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
// CHECK-MESSAGES: string:28:8: note: method 'basic_string'::empty() defined here
// CHECK-FIXES: {{^ }}if (!s.empty()){{$}}
if (s.length())
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty]
// CHECK-MESSAGES: string:28:8: note: method 'basic_string'::empty() defined here
// CHECK-FIXES: {{^ }}if (!s.empty()){{$}}
}

void g() {
Expand Down Expand Up @@ -757,7 +784,7 @@ bool testArraySize(const Array& value) {
return value.size() == 0U;
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
// CHECK-FIXES: {{^ }}return value.empty();{{$}}
// CHECK-MESSAGES: :735:8: note: method 'array'::empty() defined here
// CHECK-MESSAGES: :[[@LINE-25]]:8: note: method 'array'::empty() defined here
}

bool testArrayCompareToEmpty(const Array& value) {
Expand All @@ -768,7 +795,7 @@ bool testDummyType(const DummyType& value) {
return value == DummyType();
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used to check for emptiness instead of comparing to an empty object [readability-container-size-empty]
// CHECK-FIXES: {{^ }}return value.empty();{{$}}
// CHECK-MESSAGES: :745:8: note: method 'DummyType'::empty() defined here
// CHECK-MESSAGES: :[[@LINE-26]]:8: note: method 'DummyType'::empty() defined here
}

bool testIgnoredDummyType(const IgnoredDummyType& value) {
Expand Down

0 comments on commit efebb4e

Please sign in to comment.