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.
http://reviews.llvm.org/D6925
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/ContainerSizeEmpty.cpp
clang-tidy/readability/ContainerSizeEmpty.h
clang-tidy/readability/ReadabilityTidyModule.cpp
test/clang-tidy/readibility-container-size-empty.cpp
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
Index: clang-tidy/readability/CMakeLists.txt
===================================================================
--- clang-tidy/readability/CMakeLists.txt
+++ clang-tidy/readability/CMakeLists.txt
@@ -2,6 +2,7 @@
add_clang_library(clangTidyReadabilityModule
BracesAroundStatementsCheck.cpp
+ ContainerSizeEmpty.cpp
FunctionSize.cpp
NamespaceCommentCheck.cpp
ReadabilityTidyModule.cpp
Index: clang-tidy/readability/ContainerSizeEmpty.cpp
===================================================================
--- clang-tidy/readability/ContainerSizeEmpty.cpp
+++ clang-tidy/readability/ContainerSizeEmpty.cpp
@@ -0,0 +1,174 @@
+//===--- 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 {
+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;
+}();
+
+bool isContainer(llvm::StringRef className) {
+ 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 wrong_use = 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"))), wrong_use).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.
+ const auto UO = Result.Nodes.getNodeAs<UnaryOperator>("NegOnSize");
+ if (UO)
+ Hint =
+ FixItHint::CreateReplacement(UO->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
Index: clang-tidy/readability/ContainerSizeEmpty.h
===================================================================
--- clang-tidy/readability/ContainerSizeEmpty.h
+++ clang-tidy/readability/ContainerSizeEmpty.h
@@ -0,0 +1,39 @@
+//===--- 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 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.
+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
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===================================================================
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -11,6 +11,7 @@
#include "../ClangTidyModule.h"
#include "../ClangTidyModuleRegistry.h"
#include "BracesAroundStatementsCheck.h"
+#include "ContainerSizeEmpty.h"
#include "FunctionSize.h"
#include "RedundantSmartptrGet.h"
@@ -23,6 +24,8 @@
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
CheckFactories.registerCheck<BracesAroundStatementsCheck>(
"readability-braces-around-statements");
+ CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
+ "readability-container-size-empty");
CheckFactories.registerCheck<FunctionSizeCheck>(
"readability-function-size");
CheckFactories.registerCheck<RedundantSmartptrGet>(
Index: test/clang-tidy/readibility-container-size-empty.cpp
===================================================================
--- test/clang-tidy/readibility-container-size-empty.cpp
+++ test/clang-tidy/readibility-container-size-empty.cpp
@@ -0,0 +1,107 @@
+// 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-1]]:5: warning: The 'empty' method should be used to check for emptiness instead of 'size'. [readability-container-size-empty]
+ // CHECK-MESSAGES: if(vect.size() == 0);
+ // CHECK-FIXES: vect.empty()
+ if(vect.size() != 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used
+ // CHECK-MESSAGES: if(vect.size() != 0);
+ // CHECK-FIXES: !vect.empty()
+ if(0 == vect.size());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: The 'empty' method should be used
+ // CHECK-MESSAGES: if(0 == vect.size());
+ // CHECK-FIXES: vect.empty()
+ if(0 != vect.size());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: The 'empty' method should be used
+ // CHECK-MESSAGES: if(0 != vect.size());
+ // CHECK-FIXES: !vect.empty()
+ if(vect.size() > 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used
+ // CHECK-MESSAGES: if(vect.size() > 0);
+ // CHECK-FIXES: !vect.empty()
+ if(0 < vect.size());
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: The 'empty' method should be used
+ // CHECK-MESSAGES: if(0 < vect.size());
+ // CHECK-FIXES: !vect.empty()
+ if(vect.size() < 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used
+ // CHECK-MESSAGES: if(vect.size() < 1);
+ // CHECK-FIXES: vect.empty()
+ if(1 > vect.size());
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: The 'empty' method should be used
+ // CHECK-MESSAGES: if(1 > vect.size());
+ // CHECK-FIXES: vect.empty()
+ if(vect.size() >= 1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used
+ // CHECK-MESSAGES: if(vect.size() >= 1);
+ // CHECK-FIXES: !vect.empty()
+ if(1 <= vect.size());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: The 'empty' method should be used
+ // CHECK-MESSAGES: if(1 <= vect.size());
+ // CHECK-FIXES: !vect.empty()
+ if(!vect.size());
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: The 'empty' method should be used
+ // CHECK-MESSAGES: if(!vect.size());
+ // CHECK-FIXES: vect.empty()
+ if(vect.size());
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used
+ // CHECK-MESSAGES: if(vect.size());
+ // CHECK-FIXES: !vect.empty()
+
+ if(vect.empty());
+
+ const std::vector<int> vect2;
+ if(vect2.size() != 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used
+ // CHECK-MESSAGES: if(vect2.size() != 0)
+ // CHECK-FIXES: !vect2.empty()
+
+ std::vector<int> *vect3 = new std::vector<int>();
+ if(vect3->size() == 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used
+ // CHECK-MESSAGES: if(vect3->size() == 0)
+ // CHECK-FIXES: vect3->empty()
+
+ delete vect3;
+
+ const std::vector<int> &vect4 = vect2;
+ if(vect4.size() == 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used
+ // CHECK-MESSAGES: if(vect4.size() == 0)
+ // CHECK-FIXES: vect4.empty()
+}
+
+#define CHECKSIZE(x) if (x.size())
+
+template<typename T>
+void f() {
+ std::vector<T> v;
+ if (v.size()) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: The 'empty' method should be used
+ // CHECK-MESSAGES: if (v.size()) {}
+ // CHECK-FIXES: !v.empty()
+ 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*>();
+}
+
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
