forked from OSchip/llvm-project
[clang-tidy] Add "emplace_back" detection in inefficient-vector-operation.
Reviewers: alexfh, aaron.ballman Reviewed By: alexfh Subscribers: cfe-commits, Prazek, malcolm.parsons, xazax.hun Tags: #clang-tools-extra Differential Revision: https://reviews.llvm.org/D33209 llvm-svn: 303157
This commit is contained in:
Haojian Wu
parent
de69ff9b7c
commit
be6ef0eff9
|
|
@ -39,14 +39,14 @@ namespace {
|
|||
// - VectorVarDeclName: 'v' in (as VarDecl).
|
||||
// - VectorVarDeclStmatName: The entire 'std::vector<T> v;' statement (as
|
||||
// DeclStmt).
|
||||
// - PushBackCallName: 'v.push_back(i)' (as cxxMemberCallExpr).
|
||||
// - PushBackOrEmplaceBackCallName: 'v.push_back(i)' (as cxxMemberCallExpr).
|
||||
// - LoopInitVarName: 'i' (as VarDecl).
|
||||
// - LoopEndExpr: '10+1' (as Expr).
|
||||
static const char LoopCounterName[] = "for_loop_counter";
|
||||
static const char LoopParentName[] = "loop_parent";
|
||||
static const char VectorVarDeclName[] = "vector_var_decl";
|
||||
static const char VectorVarDeclStmtName[] = "vector_var_decl_stmt";
|
||||
static const char PushBackCallName[] = "push_back_call";
|
||||
static const char PushBackOrEmplaceBackCallName[] = "append_call";
|
||||
static const char LoopInitVarName[] = "loop_init_var";
|
||||
static const char LoopEndExprName[] = "loop_end_expr";
|
||||
|
||||
|
|
@ -81,13 +81,13 @@ void InefficientVectorOperationCheck::registerMatchers(MatchFinder *Finder) {
|
|||
const auto VectorVarDecl =
|
||||
varDecl(hasInitializer(VectorDefaultConstructorCall))
|
||||
.bind(VectorVarDeclName);
|
||||
const auto PushBackCallExpr =
|
||||
const auto VectorAppendCallExpr =
|
||||
cxxMemberCallExpr(
|
||||
callee(cxxMethodDecl(hasName("push_back"))), on(hasType(VectorDecl)),
|
||||
callee(cxxMethodDecl(hasAnyName("push_back", "emplace_back"))),
|
||||
on(hasType(VectorDecl)),
|
||||
onImplicitObjectArgument(declRefExpr(to(VectorVarDecl))))
|
||||
.bind(PushBackCallName);
|
||||
const auto PushBackCall =
|
||||
expr(anyOf(PushBackCallExpr, exprWithCleanups(has(PushBackCallExpr))));
|
||||
.bind(PushBackOrEmplaceBackCallName);
|
||||
const auto VectorAppendCall = expr(ignoringImplicit(VectorAppendCallExpr));
|
||||
const auto VectorVarDefStmt =
|
||||
declStmt(hasSingleDecl(equalsBoundNode(VectorVarDeclName)))
|
||||
.bind(VectorVarDeclStmtName);
|
||||
|
|
@ -98,9 +98,11 @@ void InefficientVectorOperationCheck::registerMatchers(MatchFinder *Finder) {
|
|||
const auto RefersToLoopVar = ignoringParenImpCasts(
|
||||
declRefExpr(to(varDecl(equalsBoundNode(LoopInitVarName)))));
|
||||
|
||||
// Matchers for the loop whose body has only 1 push_back calling statement.
|
||||
const auto HasInterestingLoopBody = hasBody(anyOf(
|
||||
compoundStmt(statementCountIs(1), has(PushBackCall)), PushBackCall));
|
||||
// Matchers for the loop whose body has only 1 push_back/emplace_back calling
|
||||
// statement.
|
||||
const auto HasInterestingLoopBody =
|
||||
hasBody(anyOf(compoundStmt(statementCountIs(1), has(VectorAppendCall)),
|
||||
VectorAppendCall));
|
||||
const auto InInterestingCompoundStmt =
|
||||
hasParent(compoundStmt(has(VectorVarDefStmt)).bind(LoopParentName));
|
||||
|
||||
|
|
@ -145,8 +147,8 @@ void InefficientVectorOperationCheck::check(
|
|||
const auto *ForLoop = Result.Nodes.getNodeAs<ForStmt>(LoopCounterName);
|
||||
const auto *RangeLoop =
|
||||
Result.Nodes.getNodeAs<CXXForRangeStmt>(RangeLoopName);
|
||||
const auto *PushBackCall =
|
||||
Result.Nodes.getNodeAs<CXXMemberCallExpr>(PushBackCallName);
|
||||
const auto *VectorAppendCall =
|
||||
Result.Nodes.getNodeAs<CXXMemberCallExpr>(PushBackOrEmplaceBackCallName);
|
||||
const auto *LoopEndExpr = Result.Nodes.getNodeAs<Expr>(LoopEndExprName);
|
||||
const auto *LoopParent = Result.Nodes.getNodeAs<CompoundStmt>(LoopParentName);
|
||||
|
||||
|
|
@ -173,7 +175,7 @@ void InefficientVectorOperationCheck::check(
|
|||
|
||||
llvm::StringRef VectorVarName = Lexer::getSourceText(
|
||||
CharSourceRange::getTokenRange(
|
||||
PushBackCall->getImplicitObjectArgument()->getSourceRange()),
|
||||
VectorAppendCall->getImplicitObjectArgument()->getSourceRange()),
|
||||
SM, Context->getLangOpts());
|
||||
|
||||
std::string ReserveStmt;
|
||||
|
|
@ -197,9 +199,11 @@ void InefficientVectorOperationCheck::check(
|
|||
ReserveStmt = (VectorVarName + ".reserve(" + LoopEndSource + ");\n").str();
|
||||
}
|
||||
|
||||
auto Diag = diag(PushBackCall->getLocStart(),
|
||||
"'push_back' is called inside a loop; "
|
||||
"consider pre-allocating the vector capacity before the loop");
|
||||
auto Diag =
|
||||
diag(VectorAppendCall->getLocStart(),
|
||||
"%0 is called inside a loop; "
|
||||
"consider pre-allocating the vector capacity before the loop")
|
||||
<< VectorAppendCall->getMethodDecl()->getDeclName();
|
||||
|
||||
if (!ReserveStmt.empty())
|
||||
Diag << FixItHint::CreateInsertion(LoopStmt->getLocStart(), ReserveStmt);
|
||||
|
|
|
|||
|
|
@ -3,8 +3,8 @@
|
|||
performance-inefficient-vector-operation
|
||||
========================================
|
||||
|
||||
Finds possible inefficient ``std::vector`` operations (e.g. ``push_back``) that
|
||||
may cause unnecessary memory reallocations.
|
||||
Finds possible inefficient ``std::vector`` operations (e.g. ``push_back``,
|
||||
``emplace_back``) that may cause unnecessary memory reallocations.
|
||||
|
||||
Currently, the check only detects following kinds of loops with a single
|
||||
statement body:
|
||||
|
|
@ -24,7 +24,7 @@ statement body:
|
|||
|
||||
* For-range loops like ``for (range-declaration : range_expression)``, the type
|
||||
of ``range_expression`` can be ``std::vector``, ``std::array``,
|
||||
``std::dequeue``, ``std::set``, ``std::unordered_set``, ``std::map``,
|
||||
``std::deque``, ``std::set``, ``std::unordered_set``, ``std::map``,
|
||||
``std::unordered_set``:
|
||||
|
||||
.. code-block:: c++
|
||||
|
|
|
|||
|
|
@ -35,6 +35,9 @@ class vector {
|
|||
explicit vector(size_type n);
|
||||
|
||||
void push_back(const T& val);
|
||||
|
||||
template <class... Args> void emplace_back(Args &&... args);
|
||||
|
||||
void reserve(size_t n);
|
||||
void resize(size_t n);
|
||||
|
||||
|
|
@ -61,205 +64,214 @@ int Op(int);
|
|||
|
||||
void f(std::vector<int>& t) {
|
||||
{
|
||||
std::vector<int> v;
|
||||
// CHECK-FIXES: v.reserve(10);
|
||||
std::vector<int> v0;
|
||||
// CHECK-FIXES: v0.reserve(10);
|
||||
for (int i = 0; i < 10; ++i)
|
||||
v.push_back(i);
|
||||
v0.push_back(i);
|
||||
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called inside a loop; consider pre-allocating the vector capacity before the loop
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
// CHECK-FIXES: v.reserve(10);
|
||||
std::vector<int> v1;
|
||||
// CHECK-FIXES: v1.reserve(10);
|
||||
for (int i = 0; i < 10; i++)
|
||||
v.push_back(i);
|
||||
v1.push_back(i);
|
||||
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
// CHECK-FIXES: v.reserve(10);
|
||||
std::vector<int> v2;
|
||||
// CHECK-FIXES: v2.reserve(10);
|
||||
for (int i = 0; i < 10; ++i)
|
||||
v.push_back(0);
|
||||
v2.push_back(0);
|
||||
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
// CHECK-FIXES: v.reserve(5);
|
||||
std::vector<int> v3;
|
||||
// CHECK-FIXES: v3.reserve(5);
|
||||
for (int i = 0; i < 5; ++i) {
|
||||
v.push_back(i);
|
||||
v3.push_back(i);
|
||||
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
|
||||
}
|
||||
// CHECK-FIXES-NOT: v.reserve(10);
|
||||
// CHECK-FIXES-NOT: v3.reserve(10);
|
||||
for (int i = 0; i < 10; ++i) {
|
||||
// No fix for this loop as we encounter the prior loops.
|
||||
v.push_back(i);
|
||||
v3.push_back(i);
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
std::vector<int> v2;
|
||||
v2.reserve(3);
|
||||
// CHECK-FIXES: v.reserve(10);
|
||||
std::vector<int> v4;
|
||||
std::vector<int> v5;
|
||||
v5.reserve(3);
|
||||
// CHECK-FIXES: v4.reserve(10);
|
||||
for (int i = 0; i < 10; ++i)
|
||||
v.push_back(i);
|
||||
v4.push_back(i);
|
||||
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
// CHECK-FIXES: v.reserve(t.size());
|
||||
std::vector<int> v6;
|
||||
// CHECK-FIXES: v6.reserve(t.size());
|
||||
for (std::size_t i = 0; i < t.size(); ++i) {
|
||||
v.push_back(t[i]);
|
||||
v6.push_back(t[i]);
|
||||
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
// CHECK-FIXES: v.reserve(t.size() - 1);
|
||||
std::vector<int> v7;
|
||||
// CHECK-FIXES: v7.reserve(t.size() - 1);
|
||||
for (std::size_t i = 0; i < t.size() - 1; ++i) {
|
||||
v.push_back(t[i]);
|
||||
v7.push_back(t[i]);
|
||||
} // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
// CHECK-FIXES: v.reserve(t.size());
|
||||
std::vector<int> v8;
|
||||
// CHECK-FIXES: v8.reserve(t.size());
|
||||
for (const auto &e : t) {
|
||||
v.push_back(e);
|
||||
v8.push_back(e);
|
||||
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
// CHECK-FIXES: v.reserve(t.size());
|
||||
std::vector<int> v9;
|
||||
// CHECK-FIXES: v9.reserve(t.size());
|
||||
for (const auto &e : t) {
|
||||
v.push_back(Op(e));
|
||||
v9.push_back(Op(e));
|
||||
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<Foo> v;
|
||||
// CHECK-FIXES: v.reserve(t.size());
|
||||
std::vector<Foo> v10;
|
||||
// CHECK-FIXES: v10.reserve(t.size());
|
||||
for (const auto &e : t) {
|
||||
v.push_back(Foo(e));
|
||||
v10.push_back(Foo(e));
|
||||
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<Bar> v;
|
||||
// CHECK-FIXES: v.reserve(t.size());
|
||||
std::vector<Bar> v11;
|
||||
// CHECK-FIXES: v11.reserve(t.size());
|
||||
for (const auto &e : t) {
|
||||
v.push_back(e);
|
||||
v11.push_back(e);
|
||||
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<Foo> v12;
|
||||
// CHECK-FIXES: v12.reserve(t.size());
|
||||
for (const auto &e : t) {
|
||||
v12.emplace_back(e);
|
||||
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'emplace_back' is called
|
||||
}
|
||||
}
|
||||
|
||||
// ---- Non-fixed Cases ----
|
||||
{
|
||||
std::vector<int> v;
|
||||
v.reserve(20);
|
||||
// CHECK-FIXES-NOT: v.reserve(10);
|
||||
std::vector<int> z0;
|
||||
z0.reserve(20);
|
||||
// CHECK-FIXES-NOT: z0.reserve(10);
|
||||
// There is a "reserve" call already.
|
||||
for (int i = 0; i < 10; ++i) {
|
||||
v.push_back(i);
|
||||
z0.push_back(i);
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
v.reserve(5);
|
||||
// CHECK-FIXES-NOT: v.reserve(10);
|
||||
std::vector<int> z1;
|
||||
z1.reserve(5);
|
||||
// CHECK-FIXES-NOT: z1.reserve(10);
|
||||
// There is a "reserve" call already.
|
||||
for (int i = 0; i < 10; ++i) {
|
||||
v.push_back(i);
|
||||
z1.push_back(i);
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
v.resize(5);
|
||||
// CHECK-FIXES-NOT: v.reserve(10);
|
||||
std::vector<int> z2;
|
||||
z2.resize(5);
|
||||
// CHECK-FIXES-NOT: z2.reserve(10);
|
||||
// There is a ref usage of v before the loop.
|
||||
for (int i = 0; i < 10; ++i) {
|
||||
v.push_back(i);
|
||||
z2.push_back(i);
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
v.push_back(0);
|
||||
// CHECK-FIXES-NOT: v.reserve(10);
|
||||
std::vector<int> z3;
|
||||
z3.push_back(0);
|
||||
// CHECK-FIXES-NOT: z3.reserve(10);
|
||||
// There is a ref usage of v before the loop.
|
||||
for (int i = 0; i < 10; ++i) {
|
||||
v.push_back(i);
|
||||
z3.push_back(i);
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
f(v);
|
||||
// CHECK-FIXES-NOT: v.reserve(10);
|
||||
// There is a ref usage of v before the loop.
|
||||
std::vector<int> z4;
|
||||
f(z4);
|
||||
// CHECK-FIXES-NOT: z4.reserve(10);
|
||||
// There is a ref usage of z4 before the loop.
|
||||
for (int i = 0; i < 10; ++i) {
|
||||
v.push_back(i);
|
||||
z4.push_back(i);
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<int> v(20);
|
||||
// CHECK-FIXES-NOT: v.reserve(10);
|
||||
// v is not constructed with default constructor.
|
||||
std::vector<int> z5(20);
|
||||
// CHECK-FIXES-NOT: z5.reserve(10);
|
||||
// z5 is not constructed with default constructor.
|
||||
for (int i = 0; i < 10; ++i) {
|
||||
v.push_back(i);
|
||||
z5.push_back(i);
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
// CHECK-FIXES-NOT: v.reserve(10);
|
||||
std::vector<int> z6;
|
||||
// CHECK-FIXES-NOT: z6.reserve(10);
|
||||
// For-loop is not started with 0.
|
||||
for (int i = 1; i < 10; ++i) {
|
||||
v.push_back(i);
|
||||
z6.push_back(i);
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
// CHECK-FIXES-NOT: v.reserve(t.size());
|
||||
// v isn't referenced in for-loop body.
|
||||
std::vector<int> z7;
|
||||
// CHECK-FIXES-NOT: z7.reserve(t.size());
|
||||
// z7 isn't referenced in for-loop body.
|
||||
for (std::size_t i = 0; i < t.size(); ++i) {
|
||||
t.push_back(i);
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
std::vector<int> z8;
|
||||
int k;
|
||||
// CHECK-FIXES-NOT: v.reserve(10);
|
||||
// CHECK-FIXES-NOT: z8.reserve(10);
|
||||
// For-loop isn't a fixable loop.
|
||||
for (std::size_t i = 0; k < 10; ++i) {
|
||||
v.push_back(t[i]);
|
||||
z8.push_back(t[i]);
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
// CHECK-FIXES-NOT: v.reserve(i + 1);
|
||||
std::vector<int> z9;
|
||||
// CHECK-FIXES-NOT: z9.reserve(i + 1);
|
||||
// The loop end expression refers to the loop variable i.
|
||||
for (int i = 0; i < i + 1; i++)
|
||||
v.push_back(i);
|
||||
z9.push_back(i);
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
std::vector<int> z10;
|
||||
int k;
|
||||
// CHECK-FIXES-NOT: v.reserve(10);
|
||||
// CHECK-FIXES-NOT: z10.reserve(10);
|
||||
// For-loop isn't a fixable loop.
|
||||
for (std::size_t i = 0; i < 10; ++k) {
|
||||
v.push_back(t[i]);
|
||||
z10.push_back(t[i]);
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
std::vector<int> z11;
|
||||
// initializer_list should not trigger the check.
|
||||
for (int e : {1, 2, 3, 4, 5}) {
|
||||
v.push_back(e);
|
||||
z11.push_back(e);
|
||||
}
|
||||
}
|
||||
{
|
||||
std::vector<int> v;
|
||||
std::vector<int>* v2 = &t;
|
||||
std::vector<int> z12;
|
||||
std::vector<int>* z13 = &t;
|
||||
// We only support detecting the range init expression which references
|
||||
// container directly.
|
||||
// Complex range init expressions like `*v2` is not supported.
|
||||
for (const auto &e : *v2) {
|
||||
v.push_back(e);
|
||||
// Complex range init expressions like `*z13` is not supported.
|
||||
for (const auto &e : *z13) {
|
||||
z12.push_back(e);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue