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 <typename T> struct vector {
vector();
+ bool operator==(const vector<T>& other) const;
+ bool operator!=(const vector<T>& other) const;
unsigned long size() const;
bool empty() const;
};
@@ -9,6 +11,11 @@
template <typename T> struct basic_string {
basic_string();
+ bool operator==(const basic_string<T>& other) const;
+ bool operator!=(const basic_string<T>& other) const;
+ bool operator==(const char *) const;
+ bool operator!=(const char *) const;
+ basic_string<T> operator+(const basic_string<T>& other) const;
unsigned long size() const;
bool empty() const;
};
@@ -19,6 +26,8 @@
inline namespace __v2 {
template <typename T> struct set {
set();
+ bool operator==(const set<T>& other) const;
+ bool operator!=(const set<T>& other) const;
unsigned long size() const;
bool empty() const;
};
@@ -29,6 +38,8 @@
template <typename T>
class TemplatedContainer {
public:
+ bool operator==(const TemplatedContainer<T>& other) const;
+ bool operator!=(const TemplatedContainer<T>& other) const;
int size() const;
bool empty() const;
};
@@ -36,6 +47,8 @@
template <typename T>
class PrivateEmpty {
public:
+ bool operator==(const PrivateEmpty<T>& other) const;
+ bool operator!=(const PrivateEmpty<T>& 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<int> 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<int>'::empty() defined here
+ // CHECK-MESSAGES: :32:8: note: method 'set<int>'::empty() defined here
+ if (intSet == std::set<int>())
+ ;
+ // 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<int>'::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<int> 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<int>())
+ ;
+ // 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<int>())
+ ;
+ // 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 be used
@@ -113,6 +164,14 @@
;
// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (!vect.empty()){{$}}
+ if (std::vector<int>() == vect)
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (vect.empty()){{$}}
+ if (std::vector<int>() != vect)
+ ;
+ // 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
@@ -172,6 +231,14 @@
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if ((*vect3).empty()){{$}}
+ if ((*vect3) == std::vector<int>())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if ((vect3)->empty()){{$}}
+ if (*vect3 == std::vector<int>())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if ((vect3)->empty()){{$}}
delete vect3;
@@ -180,6 +247,10 @@
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (vect4.empty()){{$}}
+ if (vect4 == std::vector<int>())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (vect4.empty()){{$}}
TemplatedContainer<void> templated_container;
if (templated_container.size() == 0)
@@ -186,18 +257,34 @@
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (templated_container.empty()){{$}}
+ if (templated_container == TemplatedContainer<void>())
+ ;
+ // 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 (templated_container != TemplatedContainer<void>())
+ ;
+ // 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 (TemplatedContainer<void>() == templated_container)
+ ;
+ // 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 (TemplatedContainer<void>() != templated_container)
+ ;
+ // 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
@@ -246,12 +333,20 @@
PrivateEmpty<void> private_empty;
if (private_empty.size() == 0)
;
+ if (private_empty == PrivateEmpty<void>())
+ ;
if (private_empty.size() != 0)
;
+ if (private_empty != PrivateEmpty<void>())
+ ;
if (0 == private_empty.size())
;
+ if (PrivateEmpty<void>() == private_empty)
+ ;
if (0 != private_empty.size())
;
+ if (PrivateEmpty<void>() != private_empty)
+ ;
if (private_empty.size() > 0)
;
if (0 < private_empty.size())
@@ -290,6 +385,10 @@
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (derived.empty()){{$}}
+ if (derived == Derived())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (derived.empty()){{$}}
}
#define CHECKSIZE(x) if (x.size())
@@ -301,6 +400,10 @@
;
// 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()){{$}}
+ if (v == std::vector<T>())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of comparing to an empty object. [readability-container-size-empty]
+ // CHECK-FIXES: {{^ }}if (v.empty()){{$}}
// CHECK-FIXES-NEXT: ;
CHECKSIZE(v);
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: the 'empty' method should be used
@@ -311,6 +414,10 @@
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (!templated_container.empty()){{$}}
+ if (templated_container != TemplatedContainer<T>())
+ ;
+ // 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
Index: clang-tidy/readability/ContainerSizeEmptyCheck.cpp
===================================================================
--- clang-tidy/readability/ContainerSizeEmptyCheck.cpp
+++ clang-tidy/readability/ContainerSizeEmptyCheck.cpp
@@ -62,23 +62,73 @@
ofClass(equalsBoundNode("container"))))))
.bind("SizeCallExpr"),
this);
+
+ // Empty constructor matcher.
+ const auto DefaultConstructor = cxxConstructExpr(
+ hasDeclaration(cxxConstructorDecl(isDefaultConstructor())));
+ // Comparison to empty string or empty constructor.
+ const auto WrongComparend = anyOf(
+ ignoringImpCasts(stringLiteral(hasSize(0))),
+ ignoringImpCasts(cxxBindTemporaryExpr(has(DefaultConstructor))),
+ ignoringImplicit(DefaultConstructor),
+ cxxConstructExpr(
+ hasDeclaration(cxxConstructorDecl(isCopyConstructor())),
+ has(expr(ignoringImpCasts(DefaultConstructor)))),
+ cxxConstructExpr(
+ hasDeclaration(cxxConstructorDecl(isMoveConstructor())),
+ has(expr(ignoringImpCasts(DefaultConstructor)))));
+ // Match the object being compared.
+ const auto STLArg =
+ anyOf(unaryOperator(
+ hasOperatorName("*"),
+ hasUnaryOperand(
+ expr(hasType(pointsTo(ValidContainer))).bind("Pointee"))),
+ expr(hasType(ValidContainer)).bind("STLObject"));
+ const auto STLOuter = anyOf(allOf(ignoringImplicit(declRefExpr().bind("Decl")),
+ STLArg),
+ STLArg);
+ Finder->addMatcher(
+ cxxOperatorCallExpr(
+ anyOf(hasOverloadedOperatorName("=="),
+ hasOverloadedOperatorName("!=")),
+ anyOf(allOf(hasArgument(0, WrongComparend), hasArgument(1, STLOuter)),
+ allOf(hasArgument(0, STLOuter), hasArgument(1, WrongComparend))),
+ unless(hasAncestor(
+ cxxMethodDecl(ofClass(equalsBoundNode("container"))))))
+ .bind("BinCmp"),
+ this);
}
-
void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
const auto *MemberCall =
Result.Nodes.getNodeAs<CXXMemberCallExpr>("SizeCallExpr");
+ const auto *BinCmp = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("BinCmp");
const auto *BinaryOp = Result.Nodes.getNodeAs<BinaryOperator>("SizeBinaryOp");
- const auto *E = MemberCall->getImplicitObjectArgument();
+ const auto *Pointee = Result.Nodes.getNodeAs<Expr>("Pointee");
+ const auto *Decl = Result.Nodes.getNodeAs<DeclRefExpr>("Decl");
+ const auto *E =
+ MemberCall
+ ? MemberCall->getImplicitObjectArgument()
+ : (Pointee ? Pointee : Result.Nodes.getNodeAs<Expr>("STLObject"));
FixItHint Hint;
std::string ReplacementText =
Lexer::getSourceText(CharSourceRange::getTokenRange(E->getSourceRange()),
*Result.SourceManager, getLangOpts());
+ if (BinCmp && !Decl) {
+ // Not just a DeclRefExpr, so parenthesize to be on the safe side.
+ ReplacementText = "(" + ReplacementText + ")";
+ }
if (E->getType()->isPointerType())
ReplacementText += "->empty()";
else
ReplacementText += ".empty()";
- if (BinaryOp) { // Determine the correct transformation.
+ if (BinCmp) {
+ if (BinCmp->getOperator() == OO_ExclaimEqual) {
+ ReplacementText = "!" + ReplacementText;
+ }
+ Hint =
+ FixItHint::CreateReplacement(BinCmp->getSourceRange(), ReplacementText);
+ } else if (BinaryOp) { // Determine the correct transformation.
bool Negation = false;
const bool ContainerIsLHS =
!llvm::isa<IntegerLiteral>(BinaryOp->getLHS()->IgnoreImpCasts());
@@ -148,9 +198,17 @@
"!" + ReplacementText);
}
- diag(MemberCall->getLocStart(), "the 'empty' method should be used to check "
- "for emptiness instead of 'size'")
- << Hint;
+ if (MemberCall) {
+ diag(MemberCall->getLocStart(),
+ "the 'empty' method should be used to check "
+ "for emptiness instead of 'size'")
+ << Hint;
+ } else {
+ diag(BinCmp->getLocStart(),
+ "the 'empty' method should be used to check "
+ "for emptiness instead of comparing to an empty object.")
+ << Hint;
+ }
const auto *Container = Result.Nodes.getNodeAs<NamedDecl>("container");
const auto *Empty = Result.Nodes.getNodeAs<FunctionDecl>("empty");
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
