Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a reviewer: aaron.ballman.
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:33
@@ +32,3 @@
+ const auto validContainer = namedDecl(
+ has(functionDecl(isPublic(), hasName("size"), returns(isInteger(,
+ has(functionDecl(isPublic(), hasName("empty"), returns(booleanType();
While it's not entirely unreasonable, it makes me uneasy to assume that
anything with a size() and empty() member function must be a container. Is
there a way to silence false positives aside from disabling the check entirely?
Perhaps this should instead be a user-configurable list of containers that's
prepopulated with STL container names?
Also note: `returns(isInteger())` seems a bit permissive. Consider:
```
bool size() const;
enum E { one };
enum E size() const;
```
These both will qualify for that matcher, but don't seem like particularly
valid indications that the object is a container.
https://reviews.llvm.org/D24349
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
omtcyfz updated this revision to Diff 70717.
omtcyfz added a comment.
Blacklist `enum` and `bool` return types for `size()`.
https://reviews.llvm.org/D24349
Files:
clang-tidy/readability/ContainerSizeEmptyCheck.cpp
test/clang-tidy/readability-container-size-empty.cpp
Index: test/clang-tidy/readability-container-size-empty.cpp
===
--- test/clang-tidy/readability-container-size-empty.cpp
+++ test/clang-tidy/readability-container-size-empty.cpp
@@ -24,9 +24,34 @@
};
}
-
}
+template
+class TemplatedContainer {
+public:
+ int size();
+ bool empty();
+};
+
+template
+class PrivateEmpty {
+public:
+ int size();
+private:
+ bool empty();
+};
+
+struct BoolSize {
+ bool size();
+ bool empty();
+};
+
+struct EnumSize {
+ enum E { one };
+ enum E size() const;
+ bool empty();
+};
+
int main() {
std::set intSet;
std::string str;
@@ -127,6 +152,110 @@
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (vect4.empty()){{$}}
+
+ TemplatedContainer 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 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)
+;
}
#define CHECKSIZE(x) if (x.size())
@@ -142,6 +271,16 @@
CHECKSIZE(v);
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
// CHECK-MESSAGES: CHECKSIZE(v);
+
+ TemplatedContainer templated_container;
+ if (templated_container.size())
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
+ // CH
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
omtcyfz marked an inline comment as done.
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:33
@@ +32,3 @@
+ const auto validContainer = namedDecl(
+ has(functionDecl(
+ isPublic(), hasName("size"), returns(isInteger()),
Thank you!
Blacklisted these types.
Actually, I believe if someone has `bool size()` in their codebase there's
still a problem...
https://reviews.llvm.org/D24349
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
Eugene.Zelenko added a comment. Probably check should have options to extend list of containers and also to assume all classes with integer type size() const and bool empty() const as containers. It may be not trivial to find out all custom containers and last option will be helpful to assemble such list. https://reviews.llvm.org/D24349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
aaron.ballman added a comment.
In https://reviews.llvm.org/D24349#537350, @Eugene.Zelenko wrote:
> Probably check should have options to extend list of containers and also to
> assume all classes with integer type size() const and bool empty() const as
> containers. It may be not trivial to find out all custom containers and last
> option will be helpful to assemble such list.
I think this should actually have two options: one is a list of default-checked
containers (default is the list of STL containers we previously had), and the
other is a Boolean option for guessing at what might be a container based on
function signature (default is true). This gives people a way to silence the
diagnostic while still being useful. I would document what function signatures
we use as our heuristic, but note that the function signatures we care about
may change (for instance, we may decide that const-qualification is
unimportant, or that we want to require begin()/end() functions, etc). I think
` size() const` and `bool empty() const` are a
reasonable heuristic for a first pass.
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:33
@@ +32,3 @@
+ const auto validContainer = namedDecl(
+ has(functionDecl(
+ isPublic(), hasName("size"), returns(isInteger()),
omtcyfz wrote:
> Thank you!
>
> Blacklisted these types.
>
> Actually, I believe if someone has `bool size()` in their codebase there's
> still a problem...
> Blacklisted these types.
I'm still not certain this is correct. For instance, if someone wants to do
`uint8_t size() const;`, we shouldn't punish them because `uint8_t` happens to
be `unsigned char` for their platform. But you are correct that we don't want
this to work with char32_t, for instance.
I think the best approach is to make a local matcher for the type predicate. We
want isInteger() unless it's char (that's not explicitly qualified with signed
or unsigned), bool, wchar_t, char16_t, char32_t, or an enumeration type.
> Actually, I believe if someone has bool size() in their codebase there's
> still a problem...
Agreed, though not as part of this check. ;-)
https://reviews.llvm.org/D24349
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
Eugene.Zelenko added a comment. Should we also check for absence of parameters in size() and empty() as well as const? https://reviews.llvm.org/D24349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
aaron.ballman added a comment. In https://reviews.llvm.org/D24349#537549, @Eugene.Zelenko wrote: > Should we also check for absence of parameters in size() and empty() as well > as const? I think that would be reasonable. https://reviews.llvm.org/D24349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
omtcyfz marked an inline comment as done. omtcyfz added a comment. In https://reviews.llvm.org/D24349#537350, @Eugene.Zelenko wrote: > Probably check should have options to extend list of containers and also to > assume all classes with integer type size() const and bool empty() const as > containers. It may be not trivial to find out all custom containers and last > option will be helpful to assemble such list. I was thinking about In https://reviews.llvm.org/D24349#537552, @aaron.ballman wrote: > In https://reviews.llvm.org/D24349#537549, @Eugene.Zelenko wrote: > > > Should we also check for absence of parameters in size() and empty() as > > well as const? > > > I think that would be reasonable. I do not agree about the const part. It should be encouraged to use const for these, but I do not think they should be a requirement. Repository: rL LLVM https://reviews.llvm.org/D24349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
Eugene.Zelenko added a comment. If size() and empty() change object's state, it may be not equivalent replacement. Repository: rL LLVM https://reviews.llvm.org/D24349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
omtcyfz added a comment. In https://reviews.llvm.org/D24349#537589, @Eugene.Zelenko wrote: > If size() and empty() change object's state, it may be not equivalent > replacement. True. But my point is that they are not required to do that if they're just not marked `const`. Repository: rL LLVM https://reviews.llvm.org/D24349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
xazax.hun added a comment. In https://reviews.llvm.org/D24349#537594, @omtcyfz wrote: > In https://reviews.llvm.org/D24349#537589, @Eugene.Zelenko wrote: > > > If size() and empty() change object's state, it may be not equivalent > > replacement. > > > True. But my point is that they are not required to do that if they're just > not marked `const`. I agree with Eugene. But I think a good consensus could be to warn on the non-const case but do not do the rewrite. What do you think? Repository: rL LLVM https://reviews.llvm.org/D24349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
aaron.ballman added a comment. In https://reviews.llvm.org/D24349#537595, @xazax.hun wrote: > In https://reviews.llvm.org/D24349#537594, @omtcyfz wrote: > > > In https://reviews.llvm.org/D24349#537589, @Eugene.Zelenko wrote: > > > > > If size() and empty() change object's state, it may be not equivalent > > > replacement. > > > > > > True. But my point is that they are not required to do that if they're just > > not marked `const`. > > > I agree with Eugene. But I think a good consensus could be to warn on the > non-const case but do not do the rewrite. What do you think? I think that's reasonable, depending on whether we find false positives with the warning as well (I have a slight concern about `size()` and `empty()` being unrelated operations on a non-container class that someone writes). Repository: rL LLVM https://reviews.llvm.org/D24349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
omtcyfz added a comment. In https://reviews.llvm.org/D24349#537624, @aaron.ballman wrote: > I think that's reasonable, depending on whether we find false positives with > the warning as well (I have a slight concern about `size()` and `empty()` > being unrelated operations on a non-container class that someone writes). Agreed. I'd like to try LLVM as an example of a huge codebase tomorrow to see how many false-positives there actually are. Generally, I can see the point of requiring `const` and providing options to reduce the set of allowed classes to `std::container`'s, but at the same time my thoughts are as follows: 1. Chances of people misusing C++ so much are very low. That's something I can check tomorrow just by running the check on few large open source codebases to see whether it's true. 2. If people are misusing C++ it's their fault anyway. One can do whatever he likes to do. One can rewrite all interfaces to STL containers and make `size()` and `empty()` most sophisticated functions the world has ever seen while performing whatever strange operations inside of them. Thus, I am not very supportive of the idea "let's try to think about all weird cornercases". I'm very curious to see how many false-positives there would be in LLVM codebase, for example. I'll try it tomorrow. Repository: rL LLVM https://reviews.llvm.org/D24349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
Prazek added inline comments.
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:34
@@ +33,3 @@
+ has(functionDecl(
+ isPublic(), hasName("size"), returns(isInteger()),
+ unless(anyOf(returns(isAnyCharacter()), returns(booleanType()),
Would be nice to have 'isStrictlyInteger' matcher to do this thing.
Repository:
rL LLVM
https://reviews.llvm.org/D24349
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
alexfh added a comment.
Thank you for the patch!
In https://reviews.llvm.org/D24349#537500, @aaron.ballman wrote:
> In https://reviews.llvm.org/D24349#537350, @Eugene.Zelenko wrote:
>
> > Probably check should have options to extend list of containers and also to
> > assume all classes with integer type size() const and bool empty() const as
> > containers. It may be not trivial to find out all custom containers and
> > last option will be helpful to assemble such list.
>
>
> I think this should actually have two options: one is a list of
> default-checked containers (default is the list of
> STL containers we previously had), and the other is a Boolean option for
> guessing at what might be a
> container based on function signature (default is true). This gives people a
> way to silence the diagnostic
> while still being useful.
While I understand your concerns, I would suggest to wait for actual bug
reports before introducing more options. The duck typing approach worked really
well for readability-redundant-smartptr-get and I expect it to work equally
well here. The constraints checked here are pretty strict and should make the
check safe.
> I would document what function signatures we use as our heuristic, but note
> that the function signatures we care about may change (for instance, we may
> decide that const-qualification is unimportant, or that we want to require
> begin()/end() functions, etc). I think ` size() const`
> and `bool empty() const` are a reasonable heuristic for a first pass.
Yep, documentation should be updated accordingly.
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:33
@@ +32,3 @@
+ const auto validContainer = namedDecl(
+ has(functionDecl(
+ isPublic(), hasName("size"), returns(isInteger()),
aaron.ballman wrote:
> omtcyfz wrote:
> > Thank you!
> >
> > Blacklisted these types.
> >
> > Actually, I believe if someone has `bool size()` in their codebase there's
> > still a problem...
> > Blacklisted these types.
>
> I'm still not certain this is correct. For instance, if someone wants to do
> `uint8_t size() const;`, we shouldn't punish them because `uint8_t` happens
> to be `unsigned char` for their platform. But you are correct that we don't
> want this to work with char32_t, for instance.
>
> I think the best approach is to make a local matcher for the type predicate.
> We want isInteger() unless it's char (that's not explicitly qualified with
> signed or unsigned), bool, wchar_t, char16_t, char32_t, or an enumeration
> type.
>
> > Actually, I believe if someone has bool size() in their codebase there's
> > still a problem...
>
> Agreed, though not as part of this check. ;-)
Same here, let's wait for a real user asking to support his real container
class that defines `uint8_t size() const;`.
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:145
@@ -144,3 +144,3 @@
}
diag(MemberCall->getLocStart(), "the 'empty' method should be used to check "
"for emptiness instead of 'size'")
Since we're expanding the set of supported classes, it makes sense to include
the name of the class to the diagnostic message.
Repository:
rL LLVM
https://reviews.llvm.org/D24349
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
aaron.ballman added a comment.
In https://reviews.llvm.org/D24349#538098, @alexfh wrote:
> Thank you for the patch!
>
> In https://reviews.llvm.org/D24349#537500, @aaron.ballman wrote:
>
> > In https://reviews.llvm.org/D24349#537350, @Eugene.Zelenko wrote:
> >
> > > Probably check should have options to extend list of containers and also
> > > to assume all classes with integer type size() const and bool empty()
> > > const as containers. It may be not trivial to find out all custom
> > > containers and last option will be helpful to assemble such list.
> >
> >
> > I think this should actually have two options: one is a list of
> > default-checked containers (default is the list of
> > STL containers we previously had), and the other is a Boolean option for
> > guessing at what might be a
> > container based on function signature (default is true). This gives people
> > a way to silence the diagnostic
> > while still being useful.
>
>
> While I understand your concerns, I would suggest to wait for actual bug
> reports before introducing more options. The duck typing approach worked
> really well for readability-redundant-smartptr-get and I expect it to work
> equally well here. The constraints checked here are pretty strict and should
> make the check safe.
That means there is no way to silence this check on false positives aside from
disabling it entirely, which is not a particularly good user experience.
However, I do agree that if we constrain the heuristic appropriately, the
number of false positives should be low.
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:33
@@ +32,3 @@
+ const auto validContainer = namedDecl(
+ has(functionDecl(
+ isPublic(), hasName("size"), returns(isInteger()),
alexfh wrote:
> aaron.ballman wrote:
> > omtcyfz wrote:
> > > Thank you!
> > >
> > > Blacklisted these types.
> > >
> > > Actually, I believe if someone has `bool size()` in their codebase
> > > there's still a problem...
> > > Blacklisted these types.
> >
> > I'm still not certain this is correct. For instance, if someone wants to do
> > `uint8_t size() const;`, we shouldn't punish them because `uint8_t` happens
> > to be `unsigned char` for their platform. But you are correct that we don't
> > want this to work with char32_t, for instance.
> >
> > I think the best approach is to make a local matcher for the type
> > predicate. We want isInteger() unless it's char (that's not explicitly
> > qualified with signed or unsigned), bool, wchar_t, char16_t, char32_t, or
> > an enumeration type.
> >
> > > Actually, I believe if someone has bool size() in their codebase there's
> > > still a problem...
> >
> > Agreed, though not as part of this check. ;-)
> Same here, let's wait for a real user asking to support his real container
> class that defines `uint8_t size() const;`.
I'm not certain I agree with that (I don't like adding new functionality that
is known to be imprecise, especially when dealing with heuristic-based
checking), but I don't have that strong of feelings.
Repository:
rL LLVM
https://reviews.llvm.org/D24349
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
omtcyfz removed rL LLVM as the repository for this revision.
omtcyfz updated this revision to Diff 70818.
omtcyfz added a comment.
Restricted `size()` and `empty()` functions a little bit more.
https://reviews.llvm.org/D24349
Files:
clang-tidy/readability/ContainerSizeEmptyCheck.cpp
test/clang-tidy/clang-cl-driver.cpp
test/clang-tidy/readability-container-size-empty.cpp
Index: test/clang-tidy/readability-container-size-empty.cpp
===
--- test/clang-tidy/readability-container-size-empty.cpp
+++ test/clang-tidy/readability-container-size-empty.cpp
@@ -24,9 +24,34 @@
};
}
-
}
+template
+class TemplatedContainer {
+public:
+ int size() const;
+ bool empty() const;
+};
+
+template
+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;
+};
+
int main() {
std::set intSet;
std::string str;
@@ -39,6 +64,7 @@
;
// 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-MESSAGES: :23:8: note: method 'set'::empty() defined here
if (str.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
@@ -127,6 +153,110 @@
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (vect4.empty()){{$}}
+
+ TemplatedContainer 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 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())
+;
+
+ // Typ
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
omtcyfz updated this revision to Diff 70822.
omtcyfz added a comment.
Allow inheritance for `size()` and `empty()`.
https://reviews.llvm.org/D24349
Files:
clang-tidy/readability/ContainerSizeEmptyCheck.cpp
docs/clang-tidy/checks/readability-container-size-empty.rst
test/clang-tidy/clang-cl-driver.cpp
test/clang-tidy/readability-container-size-empty.cpp
Index: test/clang-tidy/readability-container-size-empty.cpp
===
--- test/clang-tidy/readability-container-size-empty.cpp
+++ test/clang-tidy/readability-container-size-empty.cpp
@@ -24,9 +24,44 @@
};
}
-
}
+template
+class TemplatedContainer {
+public:
+ int size() const;
+ bool empty() const;
+};
+
+template
+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() {
std::set intSet;
std::string str;
@@ -39,6 +74,7 @@
;
// 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-MESSAGES: :23:8: note: method 'set'::empty() defined here
if (str.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
@@ -127,6 +163,116 @@
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (vect4.empty()){{$}}
+
+ TemplatedContainer 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 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
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.
In https://reviews.llvm.org/D24349#538102, @aaron.ballman wrote:
> In https://reviews.llvm.org/D24349#538098, @alexfh wrote:
>
> > Thank you for the patch!
> >
> > In https://reviews.llvm.org/D24349#537500, @aaron.ballman wrote:
> >
> > > In https://reviews.llvm.org/D24349#537350, @Eugene.Zelenko wrote:
> > >
> > > > Probably check should have options to extend list of containers and
> > > > also to assume all classes with integer type size() const and bool
> > > > empty() const as containers. It may be not trivial to find out all
> > > > custom containers and last option will be helpful to assemble such list.
> > >
> > >
> > > I think this should actually have two options: one is a list of
> > > default-checked containers (default is the list of
> > > STL containers we previously had), and the other is a Boolean option for
> > > guessing at what might be a
> > > container based on function signature (default is true). This gives
> > > people a way to silence the diagnostic
> > > while still being useful.
> >
> >
> > While I understand your concerns, I would suggest to wait for actual bug
> > reports before introducing more options. The duck typing approach worked
> > really well for readability-redundant-smartptr-get and I expect it to work
> > equally well here. The constraints checked here are pretty strict and
> > should make the check safe.
>
>
> That means there is no way to silence this check on false positives aside
> from disabling it entirely, which is not a particularly good user experience.
> However, I do agree that if we constrain the heuristic appropriately, the
> number of false positives should be low.
Just checked: readability-container-size-empty with the patch finds a few
thousand new warnings related to about 400 unique container names in our code.
A representative sample shows no false positives and we've found just a single
false negative (that triggers the check without the patch) - a container's
`size()` method was not `const`. Which makes me pretty sure that:
1. The new algorithm is much more consistent and useful than the whitelist
approach (gathering and maintaining a whitelist of a few hundred names is also
not fun at all).
2. Given the size of our code and the number of third-party libraries,
possibility of false positives is extremely low.
3. If there are reports of false positives, we could go with a blacklist
approach instead, but I don't think this will ever be needed.
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:33
@@ +32,3 @@
+ const auto validContainer = cxxRecordDecl(isSameOrDerivedFrom(
+ namedDecl(has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(),
+ hasName("size"), returns(isInteger()),
I don't think we can come up with a proper solution until we see a real world
example. If we want to find one faster, we can allow all character types and
wait for reports of false positives ;)
Comment at: docs/clang-tidy/checks/readability-container-size-empty.rst:18
@@ +17,3 @@
+
+Check issues warning if a container has `size()` and `empty()` methods matching
+following signatures:
`The check`
Comment at: docs/clang-tidy/checks/readability-container-size-empty.rst:18
@@ +17,3 @@
+
+Check issues warning if a container has `size()` and `empty()` methods matching
+following signatures:
alexfh wrote:
> `The check`
Double backquotes should be used.
Comment at: docs/clang-tidy/checks/readability-container-size-empty.rst:21
@@ +20,3 @@
+
+code-block::c++
+
nit: space before `c++`.
Comment at: docs/clang-tidy/checks/readability-container-size-empty.rst:23
@@ +22,3 @@
+
+ size_type size() const;
+ bool empty() const;
Explain the constraints `size_type` should satisfy.
https://reviews.llvm.org/D24349
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
omtcyfz updated this revision to Diff 70986.
omtcyfz marked 4 inline comments as done.
omtcyfz added a comment.
Address another round of comments.
https://reviews.llvm.org/D24349
Files:
clang-tidy/readability/ContainerSizeEmptyCheck.cpp
docs/clang-tidy/checks/readability-container-size-empty.rst
test/clang-tidy/clang-cl-driver.cpp
test/clang-tidy/readability-container-size-empty.cpp
Index: test/clang-tidy/readability-container-size-empty.cpp
===
--- test/clang-tidy/readability-container-size-empty.cpp
+++ test/clang-tidy/readability-container-size-empty.cpp
@@ -24,9 +24,34 @@
};
}
-
}
+template
+class TemplatedContainer {
+public:
+ int size() const;
+ bool empty() const;
+};
+
+template
+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;
+};
+
int main() {
std::set intSet;
std::string str;
@@ -39,6 +64,7 @@
;
// 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-MESSAGES: :23:8: note: method 'set'::empty() defined here
if (str.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
@@ -127,6 +153,110 @@
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (vect4.empty()){{$}}
+
+ TemplatedContainer 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 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())
+
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
omtcyfz updated this revision to Diff 70989.
omtcyfz added a comment.
Messed up with the last diff; fix that + clang-format the check.
https://reviews.llvm.org/D24349
Files:
clang-tidy/readability/ContainerSizeEmptyCheck.cpp
docs/clang-tidy/checks/readability-container-size-empty.rst
test/clang-tidy/clang-cl-driver.cpp
test/clang-tidy/readability-container-size-empty.cpp
Index: test/clang-tidy/readability-container-size-empty.cpp
===
--- test/clang-tidy/readability-container-size-empty.cpp
+++ test/clang-tidy/readability-container-size-empty.cpp
@@ -24,9 +24,43 @@
};
}
-
}
+template
+class TemplatedContainer {
+public:
+ int size() const;
+ bool empty() const;
+};
+
+template
+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() {
std::set intSet;
std::string str;
@@ -39,6 +73,7 @@
;
// 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-MESSAGES: :23:8: note: method 'set'::empty() defined here
if (str.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
@@ -127,6 +162,116 @@
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (vect4.empty()){{$}}
+
+ TemplatedContainer 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 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)
+;
+
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.
LG
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:35
@@ +34,3 @@
+ hasName("size"), returns(isInteger()),
+ unless(returns(booleanType(
+.bind("size")),
You could probably combine the two `returns` matchers. Something like this:
`returns(qualType(isInteger(), unless(booleanType(`.
https://reviews.llvm.org/D24349
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
omtcyfz updated this revision to Diff 71119.
omtcyfz marked an inline comment as done.
omtcyfz added a comment.
Combine two `returns` matchers.
https://reviews.llvm.org/D24349
Files:
clang-tidy/readability/ContainerSizeEmptyCheck.cpp
docs/clang-tidy/checks/readability-container-size-empty.rst
test/clang-tidy/readability-container-size-empty.cpp
Index: test/clang-tidy/readability-container-size-empty.cpp
===
--- test/clang-tidy/readability-container-size-empty.cpp
+++ test/clang-tidy/readability-container-size-empty.cpp
@@ -24,9 +24,43 @@
};
}
-
}
+template
+class TemplatedContainer {
+public:
+ int size() const;
+ bool empty() const;
+};
+
+template
+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() {
std::set intSet;
std::string str;
@@ -39,6 +73,7 @@
;
// 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-MESSAGES: :23:8: note: method 'set'::empty() defined here
if (str.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
@@ -127,6 +162,116 @@
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (vect4.empty()){{$}}
+
+ TemplatedContainer 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 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(
Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()
This revision was automatically updated to reflect the committed changes.
Closed by commit rL281307: [clang-tidy] Extend readability-container-size-empty
to arbitrary class with… (authored by omtcyfz).
Changed prior to commit:
https://reviews.llvm.org/D24349?vs=71119&id=71120#toc
Repository:
rL LLVM
https://reviews.llvm.org/D24349
Files:
clang-tools-extra/trunk/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-container-size-empty.rst
clang-tools-extra/trunk/test/clang-tidy/readability-container-size-empty.cpp
Index: clang-tools-extra/trunk/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
@@ -7,11 +7,11 @@
//
//===--===//
#include "ContainerSizeEmptyCheck.h"
+#include "../utils/Matchers.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Lex/Lexer.h"
#include "llvm/ADT/StringRef.h"
-#include "../utils/Matchers.h"
using namespace clang::ast_matchers;
@@ -29,11 +29,16 @@
if (!getLangOpts().CPlusPlus)
return;
- const auto stlContainer = hasAnyName(
- "array", "basic_string", "deque", "forward_list", "list", "map",
- "multimap", "multiset", "priority_queue", "queue", "set", "stack",
- "unordered_map", "unordered_multimap", "unordered_multiset",
- "unordered_set", "vector");
+ const auto validContainer = cxxRecordDecl(isSameOrDerivedFrom(
+ namedDecl(
+ has(cxxMethodDecl(
+ isConst(), parameterCountIs(0), isPublic(), hasName("size"),
+ 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(
hasParent(binaryOperator(
@@ -49,12 +54,11 @@
hasParent(explicitCastExpr(hasDestinationType(booleanType();
Finder->addMatcher(
- cxxMemberCallExpr(
- on(expr(anyOf(hasType(namedDecl(stlContainer)),
-hasType(pointsTo(namedDecl(stlContainer))),
-hasType(references(namedDecl(stlContainer)
- .bind("STLObject")),
- callee(cxxMethodDecl(hasName("size"))), WrongUse)
+ cxxMemberCallExpr(on(expr(anyOf(hasType(validContainer),
+ hasType(pointsTo(validContainer)),
+ hasType(references(validContainer
+ .bind("STLObject")),
+callee(cxxMethodDecl(hasName("size"))), WrongUse)
.bind("SizeCallExpr"),
this);
}
@@ -142,9 +146,17 @@
Hint = FixItHint::CreateReplacement(MemberCall->getSourceRange(),
"!" + ReplacementText);
}
+
diag(MemberCall->getLocStart(), "the 'empty' method should be used to check "
"for emptiness instead of 'size'")
<< Hint;
+
+ const auto *Container = Result.Nodes.getNodeAs("container");
+ const auto *Empty = Result.Nodes.getNodeAs("empty");
+
+ diag(Empty->getLocation(), "method %0::empty() defined here",
+ DiagnosticIDs::Note)
+ << Container;
}
} // namespace readability
Index: clang-tools-extra/trunk/docs/clang-tidy/checks/readability-container-size-empty.rst
===
--- clang-tools-extra/trunk/docs/clang-tidy/checks/readability-container-size-empty.rst
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/readability-container-size-empty.rst
@@ -11,6 +11,16 @@
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.
+the ``empty()`` method but not implement the ``size()`` 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:
+
+code-block:: c++
+
+ size_type size() const;
+ bool empty() const;
+
+`size_type` can be any kind of integer type.
Index: clang-tools-extra/trunk/test/clang-tidy/readability-container-size-empty.cpp
===
--- clang-tools-extra/t
