forked from OSchip/llvm-project
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! llvm-svn: 226172
This commit is contained in:
Alexander Kornienko
parent
023c806109
commit
4babd689f9
|
|
@ -2,6 +2,7 @@ set(LLVM_LINK_COMPONENTS support)
|
|||
|
||||
add_clang_library(clangTidyReadabilityModule
|
||||
BracesAroundStatementsCheck.cpp
|
||||
ContainerSizeEmpty.cpp
|
||||
ElseAfterReturnCheck.cpp
|
||||
FunctionSize.cpp
|
||||
NamespaceCommentCheck.cpp
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
@ -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
|
||||
|
|
@ -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>(
|
||||
|
|
|
|||
|
|
@ -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 *>();
|
||||
}
|
||||
Loading…
Reference in New Issue