'size' call that could be replaced with 'empty' on STL containers

[xazax.hun]

Summary

We are porting some of the checkers at a company we developed to the Clang Tidy infrastructure. We would like to open source the checkers that may be useful for the community as well. This patch is the first checker that is being ported to Clang Tidy. We also added fix-it hints, and applied them to LLVM: http://reviews.llvm.org/D6924

The code compiled and the unit tests are passed after the fixits was applied.

The documentation of the checker:

/ The emptiness of a container should be checked using the empty method
/ 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.

It also uses some custom ASTMatchers. In case you find them useful I can submit them as separate patches to clang. I will apply your suggestions to this patch.

[alexfh]

The check seems to be rather useful. Thank you for working on contributing it to ClangTidy!

The code is mostly fine, but there are a bunch of style issues. Please see the comments inline.

clang-tidy/readability/ContainerSizeEmpty.cpp

23

I'm afraid this won't compile on one of the supported toolchains: MSVS 2013. But I can't verify this myself.

Also, you could use llvm::StringSet<> instead, as it's more efficient.

30

Please use StringRef instead of "const std::string &".

87

nit: Please remove the empty line.

88

Looks like a valid use case for "auto". The type is already spelled in the initializer.

88

"MC" is too cryptic. Maybe "Call" or "MemberCall"? The variable is only used twice, so this won't increase the code size a lot.

90

Maybe "BinaryOperator" or "BinaryOp"?

93

nit: Variable names should be capitalized.

102

nit: Here and in other places: please end sentences with a period.

104

Did you mean "ConstIsLHS"?

108

"LIT" doesn't seem to be an acronym here, so I'd better rename it to "Literal" or something like that.

142

nit: I'd move the comment to the next line.

clang-tidy/readability/ContainerSizeEmpty.h

20

nits:

1. Sentences should end with a period.
2. s/that //
3. Please use Doxygen tags for function, class and variable names.

How about:

Checks whether a call to the \c size() method on a container can be replaced with a call to \c empty().

22

Please mark method names with the \c Doxygen tag:

... using the \c empty() method instead of the \c size() method.

modularize/ModuleAssistant.cpp

71 ↗

(On Diff #18016)

Please send cleanups as a separate patch.

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

19

Please make the CHECK-FIXES lines as specific as possible to avoid incorrect matches. E.g.:

// CHECK-FIXES: {{^  }}if(vect.size() == 0);

21

You can truncate the message in all the CHECK-MESSAGES lines after the first one. E.g.:

// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used

72

Please a couple of tests with templates and macros. I suspect that they aren't yet handled correctly.

E.g.:

template<typename T>
void f() {
  std::vector<T> v;
  if (v.size()) {}
}
void g() {
  f<int>();
  f<double>();
  f<char*>();
}

[xazax.hun]

Thank you for the detailed review.

I think I fixed all of the issues you mentioned. In case I forgot something I will include it in the fix in the next version of the patch.

[alexfh]

Thanks for addressing the comments. There are still some issues left. See the comments inline.

clang-tidy/readability/ContainerSizeEmpty.cpp

23

Here and in other places:

"The name should be camel case, and start with an upper case letter (e.g. Leader or Boats)."
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

101

This change made MemberCall a constant (before it was a non-const pointer to a constant). Was this intentional or you meant "const auto *MemberCall"?

clang-tidy/readability/ContainerSizeEmpty.h

24

Please use "\c size()", "\c empty()" etc. in all other locations as well.

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

17

nit: If the lack of space between "if" and "(" is not intentional, I'd add a space to make the code closer to the LLVM style. Maybe just clang-format the test file?

19

I actually meant to make the CHECK-FIXES lines more specific, not CHECK-MESSAGES (they are specific enough due to line numbers). Each type of CHECK lines is checked independently, and the location of a CHECK line doesn't matter, only their order (unless the [[@LINE]] expression is used). So essentially, fixed output is verified against this set of checks:

// CHECK-FIXES: vect.empty()
// CHECK-FIXES: !vect.empty()
// CHECK-FIXES: vect.empty()
// CHECK-FIXES: !vect.empty()
// CHECK-FIXES: !vect.empty()
// CHECK-FIXES: !vect.empty()
// CHECK-FIXES: vect.empty()
// CHECK-FIXES: vect.empty()
// CHECK-FIXES: !vect.empty()
// CHECK-FIXES: !vect.empty()
// CHECK-FIXES: vect.empty()
// CHECK-FIXES: !vect.empty()
// CHECK-FIXES: !vect2.empty()
// CHECK-FIXES: vect3->empty()
// CHECK-FIXES: vect4.empty()
// CHECK-FIXES: !v.empty()

The only thing that is verified, is that there is an ordered subset of lines in the input that match the patterns above. As you can guess, this is not particularly useful, as any of the "vect.empty()" patterns would also match "if(!vect.empty());" line, for example.

So please specify full lines of the output together with the leading "{{^ }}" (the part in double curlies is treated as a regular expression).

Thanks!

97

The interesting part is whether the replacement is performed only once or additionally for each template instantiation, in which case the code will still contain the replacement text, but will otherwise be corrupted.

100

The interesting part here is to check that the macro _definition_ is not "fixed".

[xazax.hun]

Thank you for the review. Unfortunately I am still getting used to the LLVM coding conventions. Hopefully less iteration might be required in the future. :)

[alexfh]

One comment seems to have slipped through the cracks. And one more nit.

clang-tidy/readability/ContainerSizeEmpty.cpp

23

It looks like this variable would better be a static local variable inside isContainer.

That's a smart way to initialize a StringSet variable, btw ;)

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

19

Not sure if Phabricator failed to display the previous comment on this line or you just missed it. Repeating it here for convenience:

---

I actually meant to make the CHECK-FIXES lines more specific, not CHECK-MESSAGES (they are specific enough due to line numbers). Each type of CHECK lines is checked independently, and the location of a CHECK line doesn't matter, only their order (unless the [[@LINE]] expression is used). So essentially, fixed output is verified against this set of checks:

// CHECK-FIXES: vect.empty()
// CHECK-FIXES: !vect.empty()
// CHECK-FIXES: vect.empty()
// CHECK-FIXES: !vect.empty()
// CHECK-FIXES: !vect.empty()
// CHECK-FIXES: !vect.empty()
// CHECK-FIXES: vect.empty()
// CHECK-FIXES: vect.empty()
// CHECK-FIXES: !vect.empty()
// CHECK-FIXES: !vect.empty()
// CHECK-FIXES: vect.empty()
// CHECK-FIXES: !vect.empty()
// CHECK-FIXES: !vect2.empty()
// CHECK-FIXES: vect3->empty()
// CHECK-FIXES: vect4.empty()
// CHECK-FIXES: !v.empty()

The only thing that is verified, is that there is an ordered subset of lines in the input that match the patterns above. As you can guess, this is not particularly useful, as any of the "vect.empty()" patterns would also match "if(!vect.empty());" line, for example.

So please specify full lines of the output together with the leading "{{^ }}" (the part in double curlies is treated as a regular expression).

Thanks!

---

One clarification: I think, CHECK-MESSAGES lines checking code snippets in warning messages don't add any value and just complicate the test. You could safely remove them and instead make CHECK-FIXES lines verify whole lines.

[xazax.hun]

Thank you, it looks like I missed that comment. I have altered the test according to your recommendations. I think I get it now how those check-fixes works :)

[alexfh]

This now looks good. Thank you for putting effort into open-sourcing this check!

I still suspect that macros and templates as seen in the test are handled correctly just by coincidence, but it should be easy to fix once any problems are found on the real code.

I'll commit the patch for you.

[alexfh]

There were some changes in clang-tools-extra and the patch doesn't apply cleanly. Please rebase the patch on top of the current HEAD.

[xazax.hun]

I have rebased it to the current head. Sorry for the inconvenience.

[alexfh]

No problem.

Committed revision 226172.

clang-tidy/readability/CMakeLists.txt

 
 add_clang_library(clangTidyReadabilityModule
   BracesAroundStatementsCheck.cpp
+  ContainerSizeEmpty.cpp
   ElseAfterReturnCheck.cpp
   FunctionSize.cpp
   NamespaceCommentCheck.cpp

clang-tidy/readability/ContainerSizeEmpty.h

+//===--- ContainerSizeEmpty.h - clang-tidy ----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERSIZEEMPTY_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERSIZEEMPTY_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// \brief Checks whether a call to the \c size() method can be replaced with a
+/// call to \c empty().
+///
+/// The emptiness of a container should be checked using the \c empty() method
+/// instead of the \c size() method. It is not guaranteed that \c size() is a
+/// constant-time function, and it is generally more efficient and also shows
+/// clearer intent to use \c empty(). Furthermore some containers may implement
+/// the \c empty() method but not implement the \c size() method. Using \c
+/// empty() whenever possible makes it easier to switch to another container in
+/// the future.
+class ContainerSizeEmptyCheck : public ClangTidyCheck {
+public:
+  ContainerSizeEmptyCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERSIZEEMPTY_H

clang-tidy/readability/ContainerSizeEmpty.cpp

+//===--- ContainerSizeEmpty.cpp - clang-tidy ------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+#include "ContainerSizeEmpty.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
+
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersInternal.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace {
+bool isContainer(llvm::StringRef ClassName) {
+  static const llvm::StringSet<> ContainerNames = [] {
+    llvm::StringSet<> RetVal;
+    RetVal.insert("std::vector");
+    RetVal.insert("std::list");
+    RetVal.insert("std::array");
+    RetVal.insert("std::deque");
+    RetVal.insert("std::forward_list");
+    RetVal.insert("std::set");
+    RetVal.insert("std::map");
+    RetVal.insert("std::multiset");
+    RetVal.insert("std::multimap");
+    RetVal.insert("std::unordered_set");
+    RetVal.insert("std::unordered_map");
+    RetVal.insert("std::unordered_multiset");
+    RetVal.insert("std::unordered_multimap");
+    RetVal.insert("std::stack");
+    RetVal.insert("std::queue");
+    RetVal.insert("std::priority_queue");
+    return RetVal;
+  }();
+  return ContainerNames.find(ClassName) != ContainerNames.end();
+}
+}
+
+namespace clang {
+namespace ast_matchers {
+AST_MATCHER_P(QualType, unqualifiedType, internal::Matcher<Type>,
+              InnerMatcher) {
+  return InnerMatcher.matches(*Node, Finder, Builder);
+}
+
+AST_MATCHER(Type, isBoolType) { return Node.isBooleanType(); }
+
+AST_MATCHER(NamedDecl, stlContainer) {
+  return isContainer(Node.getQualifiedNameAsString());
+}
+}
+}
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+ContainerSizeEmptyCheck::ContainerSizeEmptyCheck(StringRef Name,
+                                                 ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context) {}
+
+void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
+  const auto WrongUse = anyOf(
+      hasParent(
+          binaryOperator(
+              anyOf(has(integerLiteral(equals(0))),
+                    allOf(anyOf(hasOperatorName("<"), hasOperatorName(">="),
+                                hasOperatorName(">"), hasOperatorName("<=")),
+                          anyOf(hasRHS(integerLiteral(equals(1))),
+                                hasLHS(integerLiteral(equals(1)))))))
+              .bind("SizeBinaryOp")),
+      hasParent(implicitCastExpr(
+          hasImplicitDestinationType(unqualifiedType(isBoolType())),
+          anyOf(
+              hasParent(unaryOperator(hasOperatorName("!")).bind("NegOnSize")),
+              anything()))),
+      hasParent(
+          explicitCastExpr(hasDestinationType(unqualifiedType(isBoolType())))));
+
+  Finder->addMatcher(
+      memberCallExpr(
+          on(expr(anyOf(hasType(namedDecl(stlContainer())),
+                        hasType(qualType(pointsTo(namedDecl(stlContainer())))),
+                        hasType(qualType(references(
+                            namedDecl(stlContainer())))))).bind("STLObject")),
+          callee(methodDecl(hasName("size"))), WrongUse).bind("SizeCallExpr"),
+      this);
+}
+
+void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MemberCall =
+      Result.Nodes.getNodeAs<CXXMemberCallExpr>("SizeCallExpr");
+  const auto *BinaryOp = Result.Nodes.getNodeAs<BinaryOperator>("SizeBinaryOp");
+  const auto *E = Result.Nodes.getNodeAs<Expr>("STLObject");
+  FixItHint Hint;
+  std::string ReplacementText =
+      Lexer::getSourceText(CharSourceRange::getTokenRange(E->getSourceRange()),
+                           *Result.SourceManager, LangOptions());
+  if (E->getType()->isPointerType())
+    ReplacementText += "->empty()";
+  else
+    ReplacementText += ".empty()";
+
+  if (BinaryOp) { // Determine the correct transformation.
+    bool Negation = false;
+    const bool ContainerIsLHS = !llvm::isa<IntegerLiteral>(BinaryOp->getLHS());
+    const auto OpCode = BinaryOp->getOpcode();
+    uint64_t Value = 0;
+    if (ContainerIsLHS) {
+      if (const auto *Literal =
+              llvm::dyn_cast<IntegerLiteral>(BinaryOp->getRHS()))
+        Value = Literal->getValue().getLimitedValue();
+      else
+        return;
+    } else {
+      Value = llvm::dyn_cast<IntegerLiteral>(BinaryOp->getLHS())
+                  ->getValue()
+                  .getLimitedValue();
+    }
+
+    // Constant that is not handled.
+    if (Value > 1)
+      return;
+
+    // Always true, no warnings for that.
+    if ((OpCode == BinaryOperatorKind::BO_GE && Value == 0 && ContainerIsLHS) ||
+        (OpCode == BinaryOperatorKind::BO_LE && Value == 0 && !ContainerIsLHS))
+      return;
+
+    if (OpCode == BinaryOperatorKind::BO_NE && Value == 0)
+      Negation = true;
+    if ((OpCode == BinaryOperatorKind::BO_GT ||
+         OpCode == BinaryOperatorKind::BO_GE) &&
+        ContainerIsLHS)
+      Negation = true;
+    if ((OpCode == BinaryOperatorKind::BO_LT ||
+         OpCode == BinaryOperatorKind::BO_LE) &&
+        !ContainerIsLHS)
+      Negation = true;
+
+    if (Negation)
+      ReplacementText = "!" + ReplacementText;
+    Hint = FixItHint::CreateReplacement(BinaryOp->getSourceRange(),
+                                        ReplacementText);
+
+  } else {
+    // If there is a conversion above the size call to bool, it is safe to just
+    // replace size with empty.
+    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'.")
+      << Hint;
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "BracesAroundStatementsCheck.h"
+#include "ContainerSizeEmpty.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSize.h"
 #include "RedundantSmartptrGet.h"

   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<BracesAroundStatementsCheck>(
         "readability-braces-around-statements");
+    CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
+        "readability-container-size-empty");
     CheckFactories.registerCheck<ElseAfterReturnCheck>(
         "readability-else-after-return");
     CheckFactories.registerCheck<FunctionSizeCheck>(

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

+// RUN: $(dirname %s)/check_clang_tidy.sh %s readability-container-size-empty %t
+// REQUIRES: shell
+
+namespace std {
+template <typename T> struct vector {
+  vector() {}
+  int size() const {}
+  bool empty() const {}
+};
+}
+
+int main() {
+  std::vector<int> vect;
+  if (vect.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 (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]]: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 (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()){{$}}
+  if (vect.size() < 1)
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (vect.empty()){{$}}
+  if (1 > vect.size())
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: The 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (vect.empty()){{$}}
+  if (vect.size() >= 1)
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (!vect.empty()){{$}}
+  if (1 <= vect.size())
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: The 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (!vect.empty()){{$}}
+  if (!vect.size())
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: The 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (vect.empty()){{$}}
+  if (vect.size())
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (!vect.empty()){{$}}
+
+  if (vect.empty())
+    ;
+
+  const std::vector<int> vect2;
+  if (vect2.size() != 0)
+    ;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used
+  // CHECK-FIXES: {{^  }}if (!vect2.empty()){{$}}
+
+  std::vector<int> *vect3 = new std::vector<int>();
+  if (vect3->size() == 0)
+    ;
+  // 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()){{$}}
+}
+
+#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
+  // 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);
+}
+
+void g() {
+  f<int>();
+  f<double>();
+  f<char *>();
+}