[clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
This patch extends readability-container-size-empty check allowing it to produce warnings not only for STL containers, but also for containers, which provide two functions matching following signatures: * `size_type size() const;` * `bool empty() const;` Where `size_type` can be any kind of integer type. This functionality was proposed in https://llvm.org/bugs/show_bug.cgi?id=26823 by Eugene Zelenko. Approval: alexfh Reviewers: alexfh, aaron.ballman, Eugene.Zelenko Subscribers: etienneb, Prazek, hokein, xazax.hun, cfe-commits Differential Revision: https://reviews.llvm.org/D24349 llvm-svn: 281307
This commit is contained in:
parent
520a18df9c
commit
acb6b35b56
|
@ -7,11 +7,11 @@
|
||||||
//
|
//
|
||||||
//===----------------------------------------------------------------------===//
|
//===----------------------------------------------------------------------===//
|
||||||
#include "ContainerSizeEmptyCheck.h"
|
#include "ContainerSizeEmptyCheck.h"
|
||||||
|
#include "../utils/Matchers.h"
|
||||||
#include "clang/AST/ASTContext.h"
|
#include "clang/AST/ASTContext.h"
|
||||||
#include "clang/ASTMatchers/ASTMatchers.h"
|
#include "clang/ASTMatchers/ASTMatchers.h"
|
||||||
#include "clang/Lex/Lexer.h"
|
#include "clang/Lex/Lexer.h"
|
||||||
#include "llvm/ADT/StringRef.h"
|
#include "llvm/ADT/StringRef.h"
|
||||||
#include "../utils/Matchers.h"
|
|
||||||
|
|
||||||
using namespace clang::ast_matchers;
|
using namespace clang::ast_matchers;
|
||||||
|
|
||||||
|
@ -29,11 +29,16 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
|
||||||
if (!getLangOpts().CPlusPlus)
|
if (!getLangOpts().CPlusPlus)
|
||||||
return;
|
return;
|
||||||
|
|
||||||
const auto stlContainer = hasAnyName(
|
const auto validContainer = cxxRecordDecl(isSameOrDerivedFrom(
|
||||||
"array", "basic_string", "deque", "forward_list", "list", "map",
|
namedDecl(
|
||||||
"multimap", "multiset", "priority_queue", "queue", "set", "stack",
|
has(cxxMethodDecl(
|
||||||
"unordered_map", "unordered_multimap", "unordered_multiset",
|
isConst(), parameterCountIs(0), isPublic(), hasName("size"),
|
||||||
"unordered_set", "vector");
|
returns(qualType(isInteger(), unless(booleanType()))))
|
||||||
|
.bind("size")),
|
||||||
|
has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(),
|
||||||
|
hasName("empty"), returns(booleanType()))
|
||||||
|
.bind("empty")))
|
||||||
|
.bind("container")));
|
||||||
|
|
||||||
const auto WrongUse = anyOf(
|
const auto WrongUse = anyOf(
|
||||||
hasParent(binaryOperator(
|
hasParent(binaryOperator(
|
||||||
|
@ -49,10 +54,9 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
|
||||||
hasParent(explicitCastExpr(hasDestinationType(booleanType()))));
|
hasParent(explicitCastExpr(hasDestinationType(booleanType()))));
|
||||||
|
|
||||||
Finder->addMatcher(
|
Finder->addMatcher(
|
||||||
cxxMemberCallExpr(
|
cxxMemberCallExpr(on(expr(anyOf(hasType(validContainer),
|
||||||
on(expr(anyOf(hasType(namedDecl(stlContainer)),
|
hasType(pointsTo(validContainer)),
|
||||||
hasType(pointsTo(namedDecl(stlContainer))),
|
hasType(references(validContainer))))
|
||||||
hasType(references(namedDecl(stlContainer)))))
|
|
||||||
.bind("STLObject")),
|
.bind("STLObject")),
|
||||||
callee(cxxMethodDecl(hasName("size"))), WrongUse)
|
callee(cxxMethodDecl(hasName("size"))), WrongUse)
|
||||||
.bind("SizeCallExpr"),
|
.bind("SizeCallExpr"),
|
||||||
|
@ -142,9 +146,17 @@ void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
|
||||||
Hint = FixItHint::CreateReplacement(MemberCall->getSourceRange(),
|
Hint = FixItHint::CreateReplacement(MemberCall->getSourceRange(),
|
||||||
"!" + ReplacementText);
|
"!" + ReplacementText);
|
||||||
}
|
}
|
||||||
|
|
||||||
diag(MemberCall->getLocStart(), "the 'empty' method should be used to check "
|
diag(MemberCall->getLocStart(), "the 'empty' method should be used to check "
|
||||||
"for emptiness instead of 'size'")
|
"for emptiness instead of 'size'")
|
||||||
<< Hint;
|
<< Hint;
|
||||||
|
|
||||||
|
const auto *Container = Result.Nodes.getNodeAs<NamedDecl>("container");
|
||||||
|
const auto *Empty = Result.Nodes.getNodeAs<FunctionDecl>("empty");
|
||||||
|
|
||||||
|
diag(Empty->getLocation(), "method %0::empty() defined here",
|
||||||
|
DiagnosticIDs::Note)
|
||||||
|
<< Container;
|
||||||
}
|
}
|
||||||
|
|
||||||
} // namespace readability
|
} // namespace readability
|
||||||
|
|
|
@ -11,6 +11,16 @@ 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
|
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
|
constant-time function, and it is generally more efficient and also shows
|
||||||
clearer intent to use ``empty()``. Furthermore some containers may implement
|
clearer intent to use ``empty()``. Furthermore some containers may implement
|
||||||
the ``empty()`` method but not implement the ``size()`` method. Using ``empty()``
|
the ``empty()`` method but not implement the ``size()`` method. Using
|
||||||
whenever possible makes it easier to switch to another container in the
|
``empty()`` whenever possible makes it easier to switch to another container in
|
||||||
future.
|
the future.
|
||||||
|
|
||||||
|
The check issues warning if a container has ``size()`` and ``empty()`` methods
|
||||||
|
matching following signatures:
|
||||||
|
|
||||||
|
code-block:: c++
|
||||||
|
|
||||||
|
size_type size() const;
|
||||||
|
bool empty() const;
|
||||||
|
|
||||||
|
`size_type` can be any kind of integer type.
|
||||||
|
|
|
@ -24,9 +24,43 @@ template <typename T> struct set {
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
template <typename T>
|
||||||
|
class TemplatedContainer {
|
||||||
|
public:
|
||||||
|
int size() const;
|
||||||
|
bool empty() const;
|
||||||
|
};
|
||||||
|
|
||||||
|
template <typename T>
|
||||||
|
class PrivateEmpty {
|
||||||
|
public:
|
||||||
|
int size() const;
|
||||||
|
private:
|
||||||
|
bool empty() const;
|
||||||
|
};
|
||||||
|
|
||||||
|
struct BoolSize {
|
||||||
|
bool size() const;
|
||||||
|
bool empty() const;
|
||||||
|
};
|
||||||
|
|
||||||
|
struct EnumSize {
|
||||||
|
enum E { one };
|
||||||
|
enum E size() const;
|
||||||
|
bool empty() const;
|
||||||
|
};
|
||||||
|
|
||||||
|
class Container {
|
||||||
|
public:
|
||||||
|
int size() const;
|
||||||
|
bool empty() const;
|
||||||
|
};
|
||||||
|
|
||||||
|
class Derived : public Container {
|
||||||
|
};
|
||||||
|
|
||||||
int main() {
|
int main() {
|
||||||
std::set<int> intSet;
|
std::set<int> intSet;
|
||||||
std::string str;
|
std::string str;
|
||||||
|
@ -39,6 +73,7 @@ int main() {
|
||||||
;
|
;
|
||||||
// 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: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
|
||||||
// CHECK-FIXES: {{^ }}if (intSet.empty()){{$}}
|
// CHECK-FIXES: {{^ }}if (intSet.empty()){{$}}
|
||||||
|
// CHECK-MESSAGES: :23:8: note: method 'set<int>'::empty() defined here
|
||||||
if (str.size() == 0)
|
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
|
||||||
|
@ -127,6 +162,116 @@ int main() {
|
||||||
;
|
;
|
||||||
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
|
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
|
||||||
// CHECK-FIXES: {{^ }}if (vect4.empty()){{$}}
|
// CHECK-FIXES: {{^ }}if (vect4.empty()){{$}}
|
||||||
|
|
||||||
|
TemplatedContainer<void> templated_container;
|
||||||
|
if (templated_container.size() == 0)
|
||||||
|
;
|
||||||
|
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
|
||||||
|
// CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}}
|
||||||
|
if (templated_container.size() != 0)
|
||||||
|
;
|
||||||
|
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
|
||||||
|
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
|
||||||
|
if (0 == templated_container.size())
|
||||||
|
;
|
||||||
|
// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty' method should be used
|
||||||
|
// CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}}
|
||||||
|
if (0 != templated_container.size())
|
||||||
|
;
|
||||||
|
// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty' method should be used
|
||||||
|
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
|
||||||
|
if (templated_container.size() > 0)
|
||||||
|
;
|
||||||
|
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
|
||||||
|
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
|
||||||
|
if (0 < templated_container.size())
|
||||||
|
;
|
||||||
|
// CHECK-MESSAGES: :[[@LINE-2]]:11: warning: the 'empty' method should be used
|
||||||
|
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
|
||||||
|
if (templated_container.size() < 1)
|
||||||
|
;
|
||||||
|
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
|
||||||
|
// CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}}
|
||||||
|
if (1 > templated_container.size())
|
||||||
|
;
|
||||||
|
// CHECK-MESSAGES: :[[@LINE-2]]:11: warning: the 'empty' method should be used
|
||||||
|
// CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}}
|
||||||
|
if (templated_container.size() >= 1)
|
||||||
|
;
|
||||||
|
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
|
||||||
|
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
|
||||||
|
if (1 <= templated_container.size())
|
||||||
|
;
|
||||||
|
// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty' method should be used
|
||||||
|
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
|
||||||
|
if (templated_container.size() > 1) // no warning
|
||||||
|
;
|
||||||
|
if (1 < templated_container.size()) // no warning
|
||||||
|
;
|
||||||
|
if (templated_container.size() <= 1) // no warning
|
||||||
|
;
|
||||||
|
if (1 >= templated_container.size()) // no warning
|
||||||
|
;
|
||||||
|
if (!templated_container.size())
|
||||||
|
;
|
||||||
|
// CHECK-MESSAGES: :[[@LINE-2]]:8: warning: the 'empty' method should be used
|
||||||
|
// CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}}
|
||||||
|
if (templated_container.size())
|
||||||
|
;
|
||||||
|
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
|
||||||
|
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
|
||||||
|
|
||||||
|
if (templated_container.empty())
|
||||||
|
;
|
||||||
|
|
||||||
|
// No warnings expected.
|
||||||
|
PrivateEmpty<void> private_empty;
|
||||||
|
if (private_empty.size() == 0)
|
||||||
|
;
|
||||||
|
if (private_empty.size() != 0)
|
||||||
|
;
|
||||||
|
if (0 == private_empty.size())
|
||||||
|
;
|
||||||
|
if (0 != private_empty.size())
|
||||||
|
;
|
||||||
|
if (private_empty.size() > 0)
|
||||||
|
;
|
||||||
|
if (0 < private_empty.size())
|
||||||
|
;
|
||||||
|
if (private_empty.size() < 1)
|
||||||
|
;
|
||||||
|
if (1 > private_empty.size())
|
||||||
|
;
|
||||||
|
if (private_empty.size() >= 1)
|
||||||
|
;
|
||||||
|
if (1 <= private_empty.size())
|
||||||
|
;
|
||||||
|
if (private_empty.size() > 1)
|
||||||
|
;
|
||||||
|
if (1 < private_empty.size())
|
||||||
|
;
|
||||||
|
if (private_empty.size() <= 1)
|
||||||
|
;
|
||||||
|
if (1 >= private_empty.size())
|
||||||
|
;
|
||||||
|
if (!private_empty.size())
|
||||||
|
;
|
||||||
|
if (private_empty.size())
|
||||||
|
;
|
||||||
|
|
||||||
|
// Types with weird size() return type.
|
||||||
|
BoolSize bs;
|
||||||
|
if (bs.size() == 0)
|
||||||
|
;
|
||||||
|
EnumSize es;
|
||||||
|
if (es.size() == 0)
|
||||||
|
;
|
||||||
|
|
||||||
|
Derived derived;
|
||||||
|
if (derived.size() == 0)
|
||||||
|
;
|
||||||
|
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
|
||||||
|
// CHECK-FIXES: {{^ }}if (derived.empty()){{$}}
|
||||||
}
|
}
|
||||||
|
|
||||||
#define CHECKSIZE(x) if (x.size())
|
#define CHECKSIZE(x) if (x.size())
|
||||||
|
@ -136,12 +281,22 @@ template <typename T> void f() {
|
||||||
std::vector<T> v;
|
std::vector<T> v;
|
||||||
if (v.size())
|
if (v.size())
|
||||||
;
|
;
|
||||||
// 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' [readability-container-size-empty]
|
||||||
// CHECK-FIXES: {{^ }}if (!v.empty()){{$}}
|
// CHECK-FIXES: {{^ }}if (!v.empty()){{$}}
|
||||||
// CHECK-FIXES-NEXT: ;
|
// CHECK-FIXES-NEXT: ;
|
||||||
CHECKSIZE(v);
|
CHECKSIZE(v);
|
||||||
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
|
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
|
||||||
// CHECK-MESSAGES: CHECKSIZE(v);
|
// CHECK-MESSAGES: CHECKSIZE(v);
|
||||||
|
|
||||||
|
TemplatedContainer<T> templated_container;
|
||||||
|
if (templated_container.size())
|
||||||
|
;
|
||||||
|
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
|
||||||
|
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
|
||||||
|
// CHECK-FIXES-NEXT: ;
|
||||||
|
CHECKSIZE(templated_container);
|
||||||
|
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
|
||||||
|
// CHECK-MESSAGES: CHECKSIZE(templated_container);
|
||||||
}
|
}
|
||||||
|
|
||||||
void g() {
|
void g() {
|
||||||
|
|
Loading…
Reference in New Issue