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/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
