Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang-tidy] readability-container-size-empty handle std::string length() #14

Closed
wants to merge 1 commit into from
+86 −43
Closed

[clang-tidy] readability-container-size-empty handle std::string length() #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 37 additions & 24 deletions clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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