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

[Alexander Kornienko]

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


================
Comment at: clang-tidy/readability/ContainerSizeEmpty.cpp:22
@@ +21,3 @@
+namespace {
+const llvm::StringSet<> containerNames = [] {
+  llvm::StringSet<> RetVal;
----------------
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

================
Comment at: clang-tidy/readability/ContainerSizeEmpty.cpp:100
@@ +99,3 @@
+void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto MemberCall =
+      Result.Nodes.getNodeAs<CXXMemberCallExpr>("SizeCallExpr");
----------------
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"?

================
Comment at: clang-tidy/readability/ContainerSizeEmpty.h:23
@@ +22,3 @@
+/// The emptiness of a container should be checked using the \c empty() method
+/// instead of the \c size() method. It is not guaranteed that size is a
+/// constant-time function, and it is generally more efficient and also shows
----------------
Please use "\c size()", "\c empty()" etc. in all other locations as well.

================
Comment at: test/clang-tidy/readibility-container-size-empty.cpp:16
@@ +15,3 @@
+	std::vector<int> vect;
+	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]
----------------
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?

================
Comment at: test/clang-tidy/readibility-container-size-empty.cpp:96
@@ +95,3 @@
+  // CHECK-MESSAGES: if (v.size()) {}
+  // CHECK-FIXES: !v.empty()
+  CHECKSIZE(v); 
----------------
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.

================
Comment at: test/clang-tidy/readibility-container-size-empty.cpp:99
@@ +98,3 @@
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: The 'empty' method should be used
+  // CHECK-MESSAGES: CHECKSIZE(v);
+}
----------------
The interesting part here is to check that the macro _definition_ is not "fixed".

================
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);
----------------
alexfh wrote:
> Please make the CHECK-FIXES lines as specific as possible to avoid incorrect matches. E.g.:
> 
>   // CHECK-FIXES: {{^  }}if(vect.size() == 0);
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!

http://reviews.llvm.org/D6925

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