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