Extend readability-container-size-empty to add comparisons to empty-state objects.

Patch by Josh Zimmerman.

llvm-svn: 301185

diff --git a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
index 2402259..24680eb 100644 (file)
--- a/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
@@ -7,6 +7,7 @@
 //
 //===----------------------------------------------------------------------===//
 #include "ContainerSizeEmptyCheck.h"
+#include "../utils/ASTUtils.h"
 #include "../utils/Matchers.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
@@ -19,6 +20,8 @@ namespace clang {
 namespace tidy {
 namespace readability {
 
+using utils::IsBinaryOrTernary;
+
 ContainerSizeEmptyCheck::ContainerSizeEmptyCheck(StringRef Name,
                                                  ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context) {}
@@ -62,23 +65,70 @@ void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
                             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"));
+  Finder->addMatcher(
+      cxxOperatorCallExpr(
+          anyOf(hasOverloadedOperatorName("=="),
+                hasOverloadedOperatorName("!=")),
+          anyOf(allOf(hasArgument(0, WrongComparend), hasArgument(1, STLArg)),
+                allOf(hasArgument(0, STLArg), 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 *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 && IsBinaryOrTernary(E)) {
+    // 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 @@ void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
                                           "!" + 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");

diff --git a/clang-tools-extra/clang-tidy/utils/ASTUtils.cpp b/clang-tools-extra/clang-tidy/utils/ASTUtils.cpp
index cedac4f..eff45b5 100644 (file)
--- a/clang-tools-extra/clang-tidy/utils/ASTUtils.cpp
+++ b/clang-tools-extra/clang-tidy/utils/ASTUtils.cpp
@@ -23,6 +23,22 @@ const FunctionDecl *getSurroundingFunction(ASTContext &Context,
       "function", match(stmt(hasAncestor(functionDecl().bind("function"))),
                         Statement, Context));
 }
+
+bool IsBinaryOrTernary(const Expr *E) {
+  const Expr *E_base = E->IgnoreImpCasts();
+  if (clang::isa<clang::BinaryOperator>(E_base) ||
+      clang::isa<clang::ConditionalOperator>(E_base)) {
+    return true;
+  }
+
+  if (const auto *Operator =
+          clang::dyn_cast<clang::CXXOperatorCallExpr>(E_base)) {
+    return Operator->isInfixBinaryOp();
+  }
+
+  return false;
+}
+
 } // namespace utils
 } // namespace tidy
 } // namespace clang

diff --git a/clang-tools-extra/clang-tidy/utils/ASTUtils.h b/clang-tools-extra/clang-tidy/utils/ASTUtils.h
index 898ba9e..7f75327 100644 (file)
--- a/clang-tools-extra/clang-tidy/utils/ASTUtils.h
+++ b/clang-tools-extra/clang-tidy/utils/ASTUtils.h
@@ -18,6 +18,8 @@ namespace utils {
 // Returns the (closest) Function declaration surrounding |Statement| or NULL.
 const FunctionDecl *getSurroundingFunction(ASTContext &Context,
                                            const Stmt &Statement);
+// Determine whether Expr is a Binary or Ternary expression.
+bool IsBinaryOrTernary(const Expr *E);
 } // namespace utils
 } // namespace tidy
 } // namespace clang

diff --git a/clang-tools-extra/test/clang-tidy/readability-container-size-empty.cpp b/clang-tools-extra/test/clang-tidy/readability-container-size-empty.cpp
index 51c2da6..3c3c9a6 100644 (file)
--- a/clang-tools-extra/test/clang-tidy/readability-container-size-empty.cpp
+++ b/clang-tools-extra/test/clang-tidy/readability-container-size-empty.cpp
@@ -3,12 +3,19 @@
 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;
 };
 
 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 @@ typedef basic_string<wchar_t> wstring;
 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> struct set {
 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 @@ public:
 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 @@ struct EnumSize {
 
 class Container {
 public:
+  bool operator==(const Container& other) const;
   int size() const;
   bool empty() const;
 };
@@ -75,9 +89,21 @@ public:
 
 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<int> intSet;
   std::string str;
+  std::string str2;
   std::wstring wstr;
   str.size() + 0;
   str.size() - 0;
@@ -87,24 +113,57 @@ int main() {
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
   // CHECK-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 (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<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 +172,14 @@ int main() {
     ;
   // 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 +239,14 @@ int main() {
     ;
   // 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,24 +255,44 @@ int main() {
     ;
   // 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)
     ;
   // 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 +341,20 @@ int main() {
   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 +393,10 @@ int main() {
     ;
   // 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 +408,10 @@ template <typename T> void f() {
     ;
   // 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 +422,10 @@ template <typename T> void f() {
     ;
   // 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