Skip to content

Commit acb6b35

Browse files
kirillbobyrevkirillbobyrev
committedSep 13, 2016
[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
1 parent 520a18d commit acb6b35

File tree

3 files changed

+194
-17
lines changed

3 files changed

+194
-17
lines changed
 

Diff for: ‎clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp

+24-12
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,11 @@
77
//
88
//===----------------------------------------------------------------------===//
99
#include "ContainerSizeEmptyCheck.h"
10+
#include "../utils/Matchers.h"
1011
#include "clang/AST/ASTContext.h"
1112
#include "clang/ASTMatchers/ASTMatchers.h"
1213
#include "clang/Lex/Lexer.h"
1314
#include "llvm/ADT/StringRef.h"
14-
#include "../utils/Matchers.h"
1515

1616
using namespace clang::ast_matchers;
1717

@@ -29,11 +29,16 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
2929
if (!getLangOpts().CPlusPlus)
3030
return;
3131

32-
const auto stlContainer = hasAnyName(
33-
"array", "basic_string", "deque", "forward_list", "list", "map",
34-
"multimap", "multiset", "priority_queue", "queue", "set", "stack",
35-
"unordered_map", "unordered_multimap", "unordered_multiset",
36-
"unordered_set", "vector");
32+
const auto validContainer = cxxRecordDecl(isSameOrDerivedFrom(
33+
namedDecl(
34+
has(cxxMethodDecl(
35+
isConst(), parameterCountIs(0), isPublic(), hasName("size"),
36+
returns(qualType(isInteger(), unless(booleanType()))))
37+
.bind("size")),
38+
has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(),
39+
hasName("empty"), returns(booleanType()))
40+
.bind("empty")))
41+
.bind("container")));
3742

3843
const auto WrongUse = anyOf(
3944
hasParent(binaryOperator(
@@ -49,12 +54,11 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
4954
hasParent(explicitCastExpr(hasDestinationType(booleanType()))));
5055

5156
Finder->addMatcher(
52-
cxxMemberCallExpr(
53-
on(expr(anyOf(hasType(namedDecl(stlContainer)),
54-
hasType(pointsTo(namedDecl(stlContainer))),
55-
hasType(references(namedDecl(stlContainer)))))
56-
.bind("STLObject")),
57-
callee(cxxMethodDecl(hasName("size"))), WrongUse)
57+
cxxMemberCallExpr(on(expr(anyOf(hasType(validContainer),
58+
hasType(pointsTo(validContainer)),
59+
hasType(references(validContainer))))
60+
.bind("STLObject")),
61+
callee(cxxMethodDecl(hasName("size"))), WrongUse)
5862
.bind("SizeCallExpr"),
5963
this);
6064
}
@@ -142,9 +146,17 @@ void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
142146
Hint = FixItHint::CreateReplacement(MemberCall->getSourceRange(),
143147
"!" + ReplacementText);
144148
}
149+
145150
diag(MemberCall->getLocStart(), "the 'empty' method should be used to check "
146151
"for emptiness instead of 'size'")
147152
<< Hint;
153+
154+
const auto *Container = Result.Nodes.getNodeAs<NamedDecl>("container");
155+
const auto *Empty = Result.Nodes.getNodeAs<FunctionDecl>("empty");
156+
157+
diag(Empty->getLocation(), "method %0::empty() defined here",
158+
DiagnosticIDs::Note)
159+
<< Container;
148160
}
149161

150162
} // namespace readability

Diff for: ‎clang-tools-extra/docs/clang-tidy/checks/readability-container-size-empty.rst

+13-3
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,16 @@ The emptiness of a container should be checked using the ``empty()`` method
1111
instead of the ``size()`` method. It is not guaranteed that ``size()`` is a
1212
constant-time function, and it is generally more efficient and also shows
1313
clearer intent to use ``empty()``. Furthermore some containers may implement
14-
the ``empty()`` method but not implement the ``size()`` method. Using ``empty()``
15-
whenever possible makes it easier to switch to another container in the
16-
future.
14+
the ``empty()`` method but not implement the ``size()`` method. Using
15+
``empty()`` whenever possible makes it easier to switch to another container in
16+
the future.
17+
18+
The check issues warning if a container has ``size()`` and ``empty()`` methods
19+
matching following signatures:
20+
21+
code-block:: c++
22+
23+
size_type size() const;
24+
bool empty() const;
25+
26+
`size_type` can be any kind of integer type.

Diff for: ‎clang-tools-extra/test/clang-tidy/readability-container-size-empty.cpp

+157-2
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,43 @@ template <typename T> struct set {
2424
};
2525
}
2626

27-
2827
}
2928

29+
template <typename T>
30+
class TemplatedContainer {
31+
public:
32+
int size() const;
33+
bool empty() const;
34+
};
35+
36+
template <typename T>
37+
class PrivateEmpty {
38+
public:
39+
int size() const;
40+
private:
41+
bool empty() const;
42+
};
43+
44+
struct BoolSize {
45+
bool size() const;
46+
bool empty() const;
47+
};
48+
49+
struct EnumSize {
50+
enum E { one };
51+
enum E size() const;
52+
bool empty() const;
53+
};
54+
55+
class Container {
56+
public:
57+
int size() const;
58+
bool empty() const;
59+
};
60+
61+
class Derived : public Container {
62+
};
63+
3064
int main() {
3165
std::set<int> intSet;
3266
std::string str;
@@ -39,6 +73,7 @@ int main() {
3973
;
4074
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
4175
// CHECK-FIXES: {{^ }}if (intSet.empty()){{$}}
76+
// CHECK-MESSAGES: :23:8: note: method 'set<int>'::empty() defined here
4277
if (str.size() == 0)
4378
;
4479
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
@@ -127,6 +162,116 @@ int main() {
127162
;
128163
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
129164
// CHECK-FIXES: {{^ }}if (vect4.empty()){{$}}
165+
166+
TemplatedContainer<void> templated_container;
167+
if (templated_container.size() == 0)
168+
;
169+
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
170+
// CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}}
171+
if (templated_container.size() != 0)
172+
;
173+
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
174+
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
175+
if (0 == templated_container.size())
176+
;
177+
// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty' method should be used
178+
// CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}}
179+
if (0 != templated_container.size())
180+
;
181+
// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty' method should be used
182+
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
183+
if (templated_container.size() > 0)
184+
;
185+
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
186+
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
187+
if (0 < templated_container.size())
188+
;
189+
// CHECK-MESSAGES: :[[@LINE-2]]:11: warning: the 'empty' method should be used
190+
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
191+
if (templated_container.size() < 1)
192+
;
193+
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
194+
// CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}}
195+
if (1 > templated_container.size())
196+
;
197+
// CHECK-MESSAGES: :[[@LINE-2]]:11: warning: the 'empty' method should be used
198+
// CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}}
199+
if (templated_container.size() >= 1)
200+
;
201+
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
202+
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
203+
if (1 <= templated_container.size())
204+
;
205+
// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty' method should be used
206+
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
207+
if (templated_container.size() > 1) // no warning
208+
;
209+
if (1 < templated_container.size()) // no warning
210+
;
211+
if (templated_container.size() <= 1) // no warning
212+
;
213+
if (1 >= templated_container.size()) // no warning
214+
;
215+
if (!templated_container.size())
216+
;
217+
// CHECK-MESSAGES: :[[@LINE-2]]:8: warning: the 'empty' method should be used
218+
// CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}}
219+
if (templated_container.size())
220+
;
221+
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
222+
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
223+
224+
if (templated_container.empty())
225+
;
226+
227+
// No warnings expected.
228+
PrivateEmpty<void> private_empty;
229+
if (private_empty.size() == 0)
230+
;
231+
if (private_empty.size() != 0)
232+
;
233+
if (0 == private_empty.size())
234+
;
235+
if (0 != private_empty.size())
236+
;
237+
if (private_empty.size() > 0)
238+
;
239+
if (0 < private_empty.size())
240+
;
241+
if (private_empty.size() < 1)
242+
;
243+
if (1 > private_empty.size())
244+
;
245+
if (private_empty.size() >= 1)
246+
;
247+
if (1 <= private_empty.size())
248+
;
249+
if (private_empty.size() > 1)
250+
;
251+
if (1 < private_empty.size())
252+
;
253+
if (private_empty.size() <= 1)
254+
;
255+
if (1 >= private_empty.size())
256+
;
257+
if (!private_empty.size())
258+
;
259+
if (private_empty.size())
260+
;
261+
262+
// Types with weird size() return type.
263+
BoolSize bs;
264+
if (bs.size() == 0)
265+
;
266+
EnumSize es;
267+
if (es.size() == 0)
268+
;
269+
270+
Derived derived;
271+
if (derived.size() == 0)
272+
;
273+
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
274+
// CHECK-FIXES: {{^ }}if (derived.empty()){{$}}
130275
}
131276

132277
#define CHECKSIZE(x) if (x.size())
@@ -136,12 +281,22 @@ template <typename T> void f() {
136281
std::vector<T> v;
137282
if (v.size())
138283
;
139-
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
284+
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
140285
// CHECK-FIXES: {{^ }}if (!v.empty()){{$}}
141286
// CHECK-FIXES-NEXT: ;
142287
CHECKSIZE(v);
143288
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
144289
// CHECK-MESSAGES: CHECKSIZE(v);
290+
291+
TemplatedContainer<T> templated_container;
292+
if (templated_container.size())
293+
;
294+
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
295+
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
296+
// CHECK-FIXES-NEXT: ;
297+
CHECKSIZE(templated_container);
298+
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
299+
// CHECK-MESSAGES: CHECKSIZE(templated_container);
145300
}
146301

147302
void g() {

0 commit comments

Comments
 (0)
Please sign in to comment.