[PATCH] D31542: [clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects
aaron.ballman closed this revision. aaron.ballman added a comment. I've commit in r301185, thank you! Repository: rL LLVM https://reviews.llvm.org/D31542 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31542: [clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects
joshz added a comment. In https://reviews.llvm.org/D31542#734809, @aaron.ballman wrote: > In https://reviews.llvm.org/D31542#734455, @joshz wrote: > > > Are there any further changes I should make, or is this good to submit now? > > > > Thanks! > > > This still LGTM, so it's good to submit. Do you still need someone to land > the patch for you? I don't have commit access, so that would be great. Thanks! Repository: rL LLVM https://reviews.llvm.org/D31542 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31542: [clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects
aaron.ballman added a comment. In https://reviews.llvm.org/D31542#734455, @joshz wrote: > Are there any further changes I should make, or is this good to submit now? > > Thanks! This still LGTM, so it's good to submit. Do you still need someone to land the patch for you? Repository: rL LLVM https://reviews.llvm.org/D31542 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31542: [clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects
joshz added a comment. Are there any further changes I should make, or is this good to submit now? Thanks! Repository: rL LLVM https://reviews.llvm.org/D31542 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31542: [clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects
joshz updated this revision to Diff 95833.
joshz marked 3 inline comments as done.
joshz added a comment.
Clean up IsBinaryOrTernary
Repository:
rL LLVM
https://reviews.llvm.org/D31542
Files:
clang-tidy/readability/ContainerSizeEmptyCheck.cpp
clang-tidy/utils/ASTUtils.cpp
clang-tidy/utils/ASTUtils.h
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
@@ -3,12 +3,19 @@
namespace std {
template struct vector {
vector();
+ bool operator==(const vector& other) const;
+ bool operator!=(const vector& other) const;
unsigned long size() const;
bool empty() const;
};
template struct basic_string {
basic_string();
+ bool operator==(const basic_string& other) const;
+ bool operator!=(const basic_string& other) const;
+ bool operator==(const char *) const;
+ bool operator!=(const char *) const;
+ basic_string operator+(const basic_string& other) const;
unsigned long size() const;
bool empty() const;
};
@@ -19,6 +26,8 @@
inline namespace __v2 {
template struct set {
set();
+ bool operator==(const set& other) const;
+ bool operator!=(const set& other) const;
unsigned long size() const;
bool empty() const;
};
@@ -29,13 +38,17 @@
template
class TemplatedContainer {
public:
+ bool operator==(const TemplatedContainer& other) const;
+ bool operator!=(const TemplatedContainer& other) const;
int size() const;
bool empty() const;
};
template
class PrivateEmpty {
public:
+ bool operator==(const PrivateEmpty& other) const;
+ bool operator!=(const PrivateEmpty& other) const;
int size() const;
private:
bool empty() const;
@@ -54,6 +67,7 @@
class Container {
public:
+ bool operator==(const Container& other) const;
int size() const;
bool empty() const;
};
@@ -75,9 +89,21 @@
bool Container3::empty() const { return this->size() == 0; }
+class Container4 {
+public:
+ bool operator==(const Container4& rhs) const;
+ int size() const;
+ bool empty() const { return *this == Container4(); }
+};
+
+std::string s_func() {
+ return std::string();
+}
+
int main() {
std::set intSet;
std::string str;
+ std::string str2;
std::wstring wstr;
str.size() + 0;
str.size() - 0;
@@ -87,32 +113,73 @@
;
// 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
+ // CHECK-MESSAGES: :32:8: note: method 'set'::empty() defined here
+ if (intSet == std::set())
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness
+ // CHECK-FIXES: {{^ }}if (intSet.empty()){{$}}
+ // CHECK-MESSAGES: :32:8: note: method 'set'::empty() defined here
+ if (s_func() == "")
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (s_func().empty()){{$}}
if (str.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (str.empty()){{$}}
+ if ((str + str2).size() == 0)
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if ((str + str2).empty()){{$}}
+ if (str == "")
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (str.empty()){{$}}
+ if (str + str2 == "")
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if ((str + str2).empty()){{$}}
if (wstr.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (wstr.empty()){{$}}
+ if (wstr == "")
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (wstr.empty()){{$}}
std::vector vect;
if (vect.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (vect.empty()){{$}}
+ if (vect == std::vector())
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (vect.empty()){{$}}
if (vect.size() != 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (!vect.empty()){{$}}
+ if (vect != std::vector())
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}}
if (0 == vect.size())
;
// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty'
[PATCH] D31542: [clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects
aaron.ballman added inline comments.
Comment at: clang-tidy/utils/ASTUtils.cpp:28
+bool IsBinaryOrTernary(const Expr *expr) {
+ if (clang::isa(expr->IgnoreImpCasts()) ||
+ clang::isa(expr->IgnoreImpCasts())) {
Rather than call `IgnoreImpCasts()` three times, you should hoist it out of the
if statements.
Comment at: clang-tidy/utils/ASTUtils.cpp:33
+
+ if (const auto* binary = clang::dyn_cast(
+ expr->IgnoreImpCasts())) {
`binary` doesn't match the usual naming conventions, and the asterisk should
bind right rather than left.
Comment at: clang-tidy/utils/ASTUtils.h:22
+// Determine whether Expr is a Binary or Ternary expression.
+bool IsBinaryOrTernary(const Expr *expr);
} // namespace utils
The parameter name doesn't match our usual naming conventions. I'd just go with
`E`.
Repository:
rL LLVM
https://reviews.llvm.org/D31542
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31542: [clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects
joshz updated this revision to Diff 95655.
joshz added a comment.
Updated per some suggestions by sbenza@ on dealing with the parentheses;
IsBinaryOrTernary is based on a function he wrote at Google.
PTAL.
Repository:
rL LLVM
https://reviews.llvm.org/D31542
Files:
clang-tidy/readability/ContainerSizeEmptyCheck.cpp
clang-tidy/utils/ASTUtils.cpp
clang-tidy/utils/ASTUtils.h
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
@@ -3,6 +3,8 @@
namespace std {
template struct vector {
vector();
+ bool operator==(const vector& other) const;
+ bool operator!=(const vector& other) const;
unsigned long size() const;
bool empty() const;
};
@@ -9,6 +11,11 @@
template struct basic_string {
basic_string();
+ bool operator==(const basic_string& other) const;
+ bool operator!=(const basic_string& other) const;
+ bool operator==(const char *) const;
+ bool operator!=(const char *) const;
+ basic_string operator+(const basic_string& other) const;
unsigned long size() const;
bool empty() const;
};
@@ -19,6 +26,8 @@
inline namespace __v2 {
template struct set {
set();
+ bool operator==(const set& other) const;
+ bool operator!=(const set& other) const;
unsigned long size() const;
bool empty() const;
};
@@ -29,6 +38,8 @@
template
class TemplatedContainer {
public:
+ bool operator==(const TemplatedContainer& other) const;
+ bool operator!=(const TemplatedContainer& other) const;
int size() const;
bool empty() const;
};
@@ -36,6 +47,8 @@
template
class PrivateEmpty {
public:
+ bool operator==(const PrivateEmpty& other) const;
+ bool operator!=(const PrivateEmpty& other) const;
int size() const;
private:
bool empty() const;
@@ -54,6 +67,7 @@
class Container {
public:
+ bool operator==(const Container& other) const;
int size() const;
bool empty() const;
};
@@ -75,9 +89,21 @@
bool Container3::empty() const { return this->size() == 0; }
+class Container4 {
+public:
+ bool operator==(const Container4& rhs) const;
+ int size() const;
+ bool empty() const { return *this == Container4(); }
+};
+
+std::string s_func() {
+ return std::string();
+}
+
int main() {
std::set intSet;
std::string str;
+ std::string str2;
std::wstring wstr;
str.size() + 0;
str.size() - 0;
@@ -87,24 +113,57 @@
;
// 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
+ // CHECK-MESSAGES: :32:8: note: method 'set'::empty() defined here
+ if (intSet == std::set())
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness
+ // CHECK-FIXES: {{^ }}if (intSet.empty()){{$}}
+ // CHECK-MESSAGES: :32:8: note: method 'set'::empty() defined here
+ if (s_func() == "")
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (s_func().empty()){{$}}
if (str.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (str.empty()){{$}}
+ if ((str + str2).size() == 0)
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if ((str + str2).empty()){{$}}
+ if (str == "")
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (str.empty()){{$}}
+ if (str + str2 == "")
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if ((str + str2).empty()){{$}}
if (wstr.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (wstr.empty()){{$}}
+ if (wstr == "")
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (wstr.empty()){{$}}
std::vector vect;
if (vect.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (vect.empty()){{$}}
+ if (vect == std::vector())
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (vect.empty()){{$}}
if (vect.size() != 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (!vect.empty()){{$}}
+ if (vect != std::vector())
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if
[PATCH] D31542: [clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects
alexfh accepted this revision. alexfh added a comment. LG with one nit. Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:209 + "the 'empty' method should be used to check " + "for emptiness instead of comparing to an empty object.") +<< Hint; nit: Please remove the trailing period to follow the style other diagnostic messages use. Repository: rL LLVM https://reviews.llvm.org/D31542 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31542: [clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects
joshz added a comment. I don't believe I have access to commit this revision myself; can someone please do it for me? Thanks! :-) Repository: rL LLVM https://reviews.llvm.org/D31542 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31542: [clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects
joshz added a comment. Thanks, Aaron! Repository: rL LLVM https://reviews.llvm.org/D31542 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31542: [clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM Repository: rL LLVM https://reviews.llvm.org/D31542 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31542: [clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects
joshz updated this revision to Diff 95488.
joshz marked an inline comment as done.
joshz edited the summary of this revision.
joshz added a comment.
Resolved the bug, with a slightly modified version of Aaron's suggestion. (It
will suggest parens for anything that wasn't just a DeclRefExpr, which may be a
bit over-exuberant, but not as much as always suggesting them.)
Repository:
rL LLVM
https://reviews.llvm.org/D31542
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
@@ -3,6 +3,8 @@
namespace std {
template struct vector {
vector();
+ bool operator==(const vector& other) const;
+ bool operator!=(const vector& other) const;
unsigned long size() const;
bool empty() const;
};
@@ -9,6 +11,11 @@
template struct basic_string {
basic_string();
+ bool operator==(const basic_string& other) const;
+ bool operator!=(const basic_string& other) const;
+ bool operator==(const char *) const;
+ bool operator!=(const char *) const;
+ basic_string operator+(const basic_string& other) const;
unsigned long size() const;
bool empty() const;
};
@@ -19,6 +26,8 @@
inline namespace __v2 {
template struct set {
set();
+ bool operator==(const set& other) const;
+ bool operator!=(const set& other) const;
unsigned long size() const;
bool empty() const;
};
@@ -29,6 +38,8 @@
template
class TemplatedContainer {
public:
+ bool operator==(const TemplatedContainer& other) const;
+ bool operator!=(const TemplatedContainer& other) const;
int size() const;
bool empty() const;
};
@@ -36,6 +47,8 @@
template
class PrivateEmpty {
public:
+ bool operator==(const PrivateEmpty& other) const;
+ bool operator!=(const PrivateEmpty& other) const;
int size() const;
private:
bool empty() const;
@@ -54,6 +67,7 @@
class Container {
public:
+ bool operator==(const Container& other) const;
int size() const;
bool empty() const;
};
@@ -75,9 +89,17 @@
bool Container3::empty() const { return this->size() == 0; }
+class Container4 {
+public:
+ bool operator==(const Container4& rhs) const;
+ int size() const;
+ bool empty() const { return *this == Container4(); }
+};
+
int main() {
std::set intSet;
std::string str;
+ std::string str2;
std::wstring wstr;
str.size() + 0;
str.size() - 0;
@@ -87,24 +109,53 @@
;
// 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
+ // CHECK-MESSAGES: :32:8: note: method 'set'::empty() defined here
+ if (intSet == std::set())
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness
+ // CHECK-FIXES: {{^ }}if (intSet.empty()){{$}}
+ // CHECK-MESSAGES: :32:8: note: method 'set'::empty() defined here
if (str.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (str.empty()){{$}}
+ if ((str + str2).size() == 0)
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if ((str + str2).empty()){{$}}
+ if (str == "")
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (str.empty()){{$}}
+ if (str + str2 == "")
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if ((str + str2).empty()){{$}}
if (wstr.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (wstr.empty()){{$}}
+ if (wstr == "")
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (wstr.empty()){{$}}
std::vector vect;
if (vect.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (vect.empty()){{$}}
+ if (vect == std::vector())
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (vect.empty()){{$}}
if (vect.size() != 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (!vect.empty()){{$}}
+ if (vect != std::vector())
+;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}}
if (0 == vect.size())
;
// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty' method should
[PATCH] D31542: [clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects
aaron.ballman added a comment.
> ... which has the wrong precedence; an extra set of parens is necessary.
> However, I don't want to add a set of parens if it isn't necessary. ...
One way to handle this is to not suggest the fixit if the container part of the
equality check is not a DeclRefExpr (perhaps after ignoring implicit casts,
etc). This means the fixit won't get suggested in some cases where it otherwise
could, such as:
std::vector v;
if (v[0] == "") {
}
but it does mean that it will still catch the common cases.
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:70
+ ignoringImpCasts(cxxBindTemporaryExpr(has(cxxConstructExpr(
+ hasDeclaration(cxxConstructorDecl(isDefaultConstructor())),
+ ignoringImplicit(cxxConstructExpr(
You should pull the
`cxxConstructExpr(hasDeclaration(cxxConstructorDecl(isDefaultConstructor(`
into a separate variable and reuse it instead of spelling it out four times.
Repository:
rL LLVM
https://reviews.llvm.org/D31542
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31542: [clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects
joshz added a comment. Hey there, reviewers. Any chance you can take a look at this change? Thanks! :-) Repository: rL LLVM https://reviews.llvm.org/D31542 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
