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

[Alexander Kornienko]

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.


================
Comment at: clang-tidy/readability/ContainerSizeEmpty.cpp:22
@@ +21,3 @@
+namespace {
+const std::set<std::string> containerNames = {
+    "std::vector", "std::list", "std::array", "std::deque", "std::forward_list",
----------------
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.

================
Comment at: clang-tidy/readability/ContainerSizeEmpty.cpp:29
@@ +28,3 @@
+
+bool isContainer(const std::string &className) {
+  return containerNames.find(className) != containerNames.end();
----------------
Please use StringRef instead of "const std::string &".

================
Comment at: clang-tidy/readability/ContainerSizeEmpty.cpp:86
@@ +85,3 @@
+void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
+
+  const CXXMemberCallExpr *MC =
----------------
nit: Please remove the empty line.

================
Comment at: clang-tidy/readability/ContainerSizeEmpty.cpp:87
@@ +86,3 @@
+
+  const CXXMemberCallExpr *MC =
+      Result.Nodes.getNodeAs<CXXMemberCallExpr>("SizeCallExpr");
----------------
Looks like a valid use case for "auto". The type is already spelled in the initializer.

================
Comment at: clang-tidy/readability/ContainerSizeEmpty.cpp:87
@@ +86,3 @@
+
+  const CXXMemberCallExpr *MC =
+      Result.Nodes.getNodeAs<CXXMemberCallExpr>("SizeCallExpr");
----------------
alexfh wrote:
> Looks like a valid use case for "auto". The type is already spelled in the initializer.
"MC" is too cryptic. Maybe "Call" or "MemberCall"? The variable is only used twice, so this won't increase the code size a lot.

================
Comment at: clang-tidy/readability/ContainerSizeEmpty.cpp:89
@@ +88,3 @@
+      Result.Nodes.getNodeAs<CXXMemberCallExpr>("SizeCallExpr");
+  const BinaryOperator *BOP =
+      Result.Nodes.getNodeAs<BinaryOperator>("SizeBinaryOp");
----------------
Maybe "BinaryOperator" or "BinaryOp"?

================
Comment at: clang-tidy/readability/ContainerSizeEmpty.cpp:92
@@ +91,3 @@
+  const Expr *E = Result.Nodes.getNodeAs<Expr>("STLObject");
+  FixItHint hint;
+  std::string ReplacementText =
----------------
nit: Variable names should be capitalized.

================
Comment at: clang-tidy/readability/ContainerSizeEmpty.cpp:101
@@ +100,3 @@
+
+  if (BOP) { // Determine the correct transformation
+    bool Negation = false;
----------------
nit: Here and in other places: please end sentences with a period.

================
Comment at: clang-tidy/readability/ContainerSizeEmpty.cpp:103
@@ +102,3 @@
+    bool Negation = false;
+    const bool ContIsLHS = !llvm::isa<IntegerLiteral>(BOP->getLHS());
+    const auto OpCode = BOP->getOpcode();
----------------
Did you mean "ConstIsLHS"?

================
Comment at: clang-tidy/readability/ContainerSizeEmpty.cpp:107
@@ +106,3 @@
+    if (ContIsLHS) {
+      if (IntegerLiteral *LIT = llvm::dyn_cast<IntegerLiteral>(BOP->getRHS()))
+        Value = LIT->getValue().getLimitedValue();
----------------
"LIT" doesn't seem to be an acronym here, so I'd better rename it to "Literal" or something like that.

================
Comment at: clang-tidy/readability/ContainerSizeEmpty.cpp:141
@@ +140,3 @@
+
+  } else { // If there is a conversion above the size call to bool, it is safe
+           // to just replace size with empty
----------------
nit: I'd move the comment to the next line.

================
Comment at: clang-tidy/readability/ContainerSizeEmpty.h:19
@@ +18,3 @@
+
+/// \brief Checks that whether a size method call can be replaced by empty
+/// 
----------------
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().

================
Comment at: clang-tidy/readability/ContainerSizeEmpty.h:21
@@ +20,3 @@
+/// 
+/// 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
----------------
Please mark method names with the \c Doxygen tag:
  ... using the \c empty() method instead of the \c size() method.

================
Comment at: modularize/ModuleAssistant.cpp:71
@@ -70,3 +70,3 @@
   // Free submodules.
-  while (SubModules.size()) {
+  while (!SubModules.empty()) {
     Module *last = SubModules.back();
----------------
Please send cleanups as a separate patch.

================
Comment at: test/clang-tidy/readibility-container-size-empty.cpp:18
@@ +17,3 @@
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used to check for emptiness instead of 'size'. [readability-container-size-empty]
+  // CHECK-FIXES: vect.empty()
+	if(vect.size() != 0);
----------------
Please make the CHECK-FIXES lines as specific as possible to avoid incorrect matches. E.g.:

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

================
Comment at: test/clang-tidy/readibility-container-size-empty.cpp:20
@@ +19,3 @@
+	if(vect.size() != 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used to check for emptiness instead of 'size'. [readability-container-size-empty]
+  // CHECK-FIXES: !vect.empty()
----------------
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

================
Comment at: test/clang-tidy/readibility-container-size-empty.cpp:71
@@ +70,2 @@
+  // CHECK-FIXES: vect4.empty()
+}
----------------
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*>();
  }

http://reviews.llvm.org/D6925

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/