[clang-tools-extra] r226172 - clang-tidy: 'size' call that could be replaced with 'empty' on STL containers
Alexander Kornienko
alexfh at google.com
Thu Jan 15 07:46:58 PST 2015
Author: alexfh
Date: Thu Jan 15 09:46:58 2015
New Revision: 226172
URL: http://llvm.org/viewvc/llvm-project?rev=226172&view=rev
Log:
clang-tidy: 'size' call that could be replaced with 'empty' on STL containers
We are porting some of the checkers at a company we developed to the Clang Tidy
infrastructure. We would like to open source the checkers that may be useful
for the community as well. This patch is the first checker that is being ported
to Clang Tidy. We also added fix-it hints, and applied them to LLVM:
http://reviews.llvm.org/D6924
The code compiled and the unit tests are passed after the fixits was applied.
The documentation of the checker:
/// 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
/// 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.
It also uses some custom ASTMatchers. In case you find them useful I can submit
them as separate patches to clang. I will apply your suggestions to this patch.
http://reviews.llvm.org/D6925
Patch by Gábor Horváth!
Added:
clang-tools-extra/trunk/clang-tidy/readability/ContainerSizeEmpty.cpp
clang-tools-extra/trunk/clang-tidy/readability/ContainerSizeEmpty.h
clang-tools-extra/trunk/test/clang-tidy/readibility-container-size-empty.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
Modified: clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt?rev=226172&r1=226171&r2=226172&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt Thu Jan 15 09:46:58 2015
@@ -2,6 +2,7 @@ set(LLVM_LINK_COMPONENTS support)
add_clang_library(clangTidyReadabilityModule
BracesAroundStatementsCheck.cpp
+ ContainerSizeEmpty.cpp
ElseAfterReturnCheck.cpp
FunctionSize.cpp
NamespaceCommentCheck.cpp
Added: clang-tools-extra/trunk/clang-tidy/readability/ContainerSizeEmpty.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/ContainerSizeEmpty.cpp?rev=226172&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/readability/ContainerSizeEmpty.cpp (added)
+++ clang-tools-extra/trunk/clang-tidy/readability/ContainerSizeEmpty.cpp Thu Jan 15 09:46:58 2015
@@ -0,0 +1,173 @@
+//===--- 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 {
+bool isContainer(llvm::StringRef ClassName) {
+ static 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;
+ }();
+ 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 WrongUse = 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"))), WrongUse).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.
+ if (const auto *UnaryOp =
+ Result.Nodes.getNodeAs<UnaryOperator>("NegOnSize"))
+ Hint = FixItHint::CreateReplacement(UnaryOp->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
Added: clang-tools-extra/trunk/clang-tidy/readability/ContainerSizeEmpty.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/ContainerSizeEmpty.h?rev=226172&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/readability/ContainerSizeEmpty.h (added)
+++ clang-tools-extra/trunk/clang-tidy/readability/ContainerSizeEmpty.h Thu Jan 15 09:46:58 2015
@@ -0,0 +1,40 @@
+//===--- 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 \c size() is a
+/// constant-time function, and it is generally more efficient and also shows
+/// clearer intent to use \c empty(). Furthermore some containers may implement
+/// the \c empty() method but not implement the \c size() method. Using \c
+/// 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
Modified: clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp?rev=226172&r1=226171&r2=226172&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp Thu Jan 15 09:46:58 2015
@@ -11,6 +11,7 @@
#include "../ClangTidyModule.h"
#include "../ClangTidyModuleRegistry.h"
#include "BracesAroundStatementsCheck.h"
+#include "ContainerSizeEmpty.h"
#include "ElseAfterReturnCheck.h"
#include "FunctionSize.h"
#include "RedundantSmartptrGet.h"
@@ -24,6 +25,8 @@ public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
CheckFactories.registerCheck<BracesAroundStatementsCheck>(
"readability-braces-around-statements");
+ CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
+ "readability-container-size-empty");
CheckFactories.registerCheck<ElseAfterReturnCheck>(
"readability-else-after-return");
CheckFactories.registerCheck<FunctionSizeCheck>(
Added: clang-tools-extra/trunk/test/clang-tidy/readibility-container-size-empty.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readibility-container-size-empty.cpp?rev=226172&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/readibility-container-size-empty.cpp (added)
+++ clang-tools-extra/trunk/test/clang-tidy/readibility-container-size-empty.cpp Thu Jan 15 09:46:58 2015
@@ -0,0 +1,106 @@
+// 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-2]]:7: warning: The 'empty' method should be used to check for emptiness instead of 'size'. [readability-container-size-empty]
+ // CHECK-FIXES: {{^ }}if (vect.empty()){{$}}
+ if (vect.size() != 0)
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}}
+ if (0 == vect.size())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: The 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (vect.empty()){{$}}
+ if (0 != vect.size())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: The 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}}
+ if (vect.size() > 0)
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}}
+ if (0 < vect.size())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: The 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}}
+ if (vect.size() < 1)
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (vect.empty()){{$}}
+ if (1 > vect.size())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: The 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (vect.empty()){{$}}
+ if (vect.size() >= 1)
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}}
+ if (1 <= vect.size())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: The 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}}
+ if (!vect.size())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: The 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (vect.empty()){{$}}
+ if (vect.size())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}}
+
+ if (vect.empty())
+ ;
+
+ const std::vector<int> vect2;
+ if (vect2.size() != 0)
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (!vect2.empty()){{$}}
+
+ std::vector<int> *vect3 = new std::vector<int>();
+ if (vect3->size() == 0)
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (vect3->empty()){{$}}
+
+ delete vect3;
+
+ const std::vector<int> &vect4 = vect2;
+ if (vect4.size() == 0)
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (vect4.empty()){{$}}
+}
+
+#define CHECKSIZE(x) if (x.size())
+// CHECK-FIXES: #define CHECKSIZE(x) if (x.size())
+
+template <typename T> void f() {
+ std::vector<T> v;
+ if (v.size())
+ ;
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used
+ // CHECK-FIXES: {{^ }}if (!v.empty()){{$}}
+ // CHECK-FIXES-NEXT: ;
+ 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 *>();
+}
More information about the cfe-commits
mailing list