[clang-tidy] Extend readability-container-size-empty to add comparisons to newly-constructed objects

[joshz]

Summary

If checking for emptiness, suggest:

if (foo.empty())

instead of

if (foo != T())  (T == decltype(foo))

or

if (foo != "")

(And similarly for the negation, and similarly for the Yoda version of these comparisons.)

[joshz]

Hey there, reviewers.
Any chance you can take a look at this change?

Thanks! :-)

[aaron.ballman]

... 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<std::string> v;
if (v[0] == "") {
}

but it does mean that it will still catch the common cases.

clang-tidy/readability/ContainerSizeEmptyCheck.cpp

70

You should pull the cxxConstructExpr(hasDeclaration(cxxConstructorDecl(isDefaultConstructor()))) into a separate variable and reuse it instead of spelling it out four times.

[joshz]

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.)

[aaron.ballman]

LGTM

[joshz]

Thanks, Aaron!

[joshz]

I don't believe I have access to commit this revision myself; can someone please do it for me?

Thanks! :-)

[alexfh]

LG with one nit.

clang-tidy/readability/ContainerSizeEmptyCheck.cpp

209

nit: Please remove the trailing period to follow the style other diagnostic messages use.

[joshz]

Updated to fix the nit as well as to take into account some suggestions by sbenza@ on dealing with the parentheses; IsBinaryOrTernary is based on a function he wrote at Google.

PTAL.

[aaron.ballman]

clang-tidy/utils/ASTUtils.cpp

28 ↗

(On Diff #95655)

Rather than call IgnoreImpCasts() three times, you should hoist it out of the if statements.

33 ↗

(On Diff #95655)

binary doesn't match the usual naming conventions, and the asterisk should bind right rather than left.

clang-tidy/utils/ASTUtils.h

22 ↗

(On Diff #95655)

The parameter name doesn't match our usual naming conventions. I'd just go with E.

[joshz]

Clean up IsBinaryOrTernary

[joshz]

Are there any further changes I should make, or is this good to submit now?

Thanks!

[aaron.ballman]

In 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?

[joshz]

In D31542#734809, @aaron.ballman wrote:

In 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!

[aaron.ballman]

I've commit in r301185, thank you!

clang-tidy/readability/ContainerSizeEmptyCheck.cpp

Finder->addMatcher(
       cxxMemberCallExpr(on(expr(anyOf(hasType(ValidContainer),
                                       hasType(pointsTo(ValidContainer)),
                                       hasType(references(ValidContainer))))),
                         callee(cxxMethodDecl(hasName("size"))), WrongUse,
                         unless(hasAncestor(cxxMethodDecl(
                             ofClass(equalsBoundNode("container"))))))
           .bind("SizeCallExpr"),
       this);
-}
 
+  // Comparison to empty string or empty constructor.
+  const auto WrongComparend = anyOf(
+      ignoringImpCasts(stringLiteral(hasSize(0))),
+      ignoringImpCasts(cxxBindTemporaryExpr(has(cxxConstructExpr(
+          hasDeclaration(cxxConstructorDecl(isDefaultConstructor())))))),
+      ignoringImplicit(cxxConstructExpr(
+          hasDeclaration(cxxConstructorDecl(isDefaultConstructor())))),
+      cxxConstructExpr(
+          hasDeclaration(cxxConstructorDecl(isCopyConstructor())),
+          has(expr(ignoringImpCasts(cxxConstructExpr(
+              hasDeclaration(cxxConstructorDecl(isDefaultConstructor()))))))),
+      cxxConstructExpr(
+          hasDeclaration(cxxConstructorDecl(isMoveConstructor())),
+          has(expr(ignoringImpCasts(cxxConstructExpr(
+              hasDeclaration(cxxConstructorDecl(isDefaultConstructor()))))))));
+  // 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 (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());
     const auto OpCode = BinaryOp->getOpcode();
     uint64_t Value = 0;
     if (ContainerIsLHS) {
       if (const auto *Literal = llvm::dyn_cast<IntegerLiteral>(
               BinaryOp->getRHS()->IgnoreImpCasts()))
if (const auto *UnaryOp =
             Result.Nodes.getNodeAs<UnaryOperator>("NegOnSize"))
       Hint = FixItHint::CreateReplacement(UnaryOp->getSourceRange(),
                                           ReplacementText);
     else
       Hint = FixItHint::CreateReplacement(MemberCall->getSourceRange(),
                                           "!" + ReplacementText);
   }
 
-  diag(MemberCall->getLocStart(), "the 'empty' method should be used to check "
-                                  "for emptiness instead of 'size'")
+  if (MemberCall) {
+    diag(MemberCall->getLocStart(),
+         "the 'empty' method should be used to check "
         << 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");
 
   diag(Empty->getLocation(), "method %0::empty() defined here",
        DiagnosticIDs::Note)
       << Container;
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

test/clang-tidy/readability-container-size-empty.cpp

 // RUN: %check_clang_tidy %s readability-container-size-empty %t
 
 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;
 };
 
 typedef basic_string<char> string;
 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;
 };
 }
 
 }
 
 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;
 };
 
 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;
 };
 
 struct BoolSize {
   bool size() const;
   bool empty() const;
 };
 
 struct EnumSize {
   enum E { one };
   enum E size() const;
   bool empty() const;
 };
 
 class Container {
 public:
+  bool operator==(const Container& other) const;
   int size() const;
   bool empty() const;
 };
 
 class Derived : public Container {
 };
 
 class Container2 {
 public:
   int size() const;
   bool empty() const { return size() == 0; }
 };
 
 class Container3 {
 public:
   int size() const;
   bool empty() const;
 };
 
 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;
   0 + str.size();
   0 - str.size();
   if (intSet.size() == 0)
     ;
   // 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: :31: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
   // CHECK-FIXES: {{^  }}if (vect.empty()){{$}}
   if (0 != vect.size())
     ;
   // 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
   // CHECK-FIXES: {{^  }}if (!vect.empty()){{$}}
   if (0 < vect.size())
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if (!vect.empty()){{$}}
int main() {
   if (vect3->size() == 0)
     ;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
   // CHECK-FIXES: {{^  }}if (vect3->empty()){{$}}
   if ((*vect3).size() == 0)
     ;
   // 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;
 
   const std::vector<int> &vect4 = vect2;
   if (vect4.size() == 0)
     ;
   // 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
   // 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()){{$}}
int main() {
 
   if (templated_container.empty())
     ;
 
   // No warnings expected.
   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())
     ;
   if (private_empty.size() < 1)
     ;
   if (1 > private_empty.size())
     ;
int main() {
   if (es.size() == 0)
     ;
 
   Derived derived;
   if (derived.size() == 0)
     ;
   // 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())
 // CHECK-FIXES: #define CHECKSIZE(x) if (x.size())
 
 template <typename T> void f() {
   std::vector<T> v;
   if (v.size())
     ;
   // 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
   // CHECK-MESSAGES: CHECKSIZE(v);
 
   TemplatedContainer<T> templated_container;
   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 != 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
   // CHECK-MESSAGES: CHECKSIZE(templated_container);
 }
 
 void g() {
   f<int>();
   f<double>();
   f<char *>();
 }