[clang-tools-extra] r300534 - [clang-tidy] Add a clang-tidy check for possible inefficient vector operations
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 18 00:46:39 PDT 2017
Author: hokein
Date: Tue Apr 18 02:46:39 2017
New Revision: 300534
URL: http://llvm.org/viewvc/llvm-project?rev=300534&view=rev
Log:
[clang-tidy] Add a clang-tidy check for possible inefficient vector operations
Summary:
The "performance-inefficient-vector-operation" check finds vector oprations in
for-loop statements which may cause multiple memory reallocations.
This is the first version, only detects typical for-loop:
```
std::vector<int> v;
for (int i = 0; i < n; ++i) {
v.push_back(i);
}
// or
for (int i = 0; i < v2.size(); ++i) {
v.push_back(v2[i]);
}
```
We can extend it to handle more cases like for-range loop in the future.
Reviewers: alexfh, aaron.ballman
Reviewed By: aaron.ballman
Subscribers: zaks.anna, Eugene.Zelenko, mgorny, cfe-commits, djasper
Differential Revision: https://reviews.llvm.org/D31757
Added:
clang-tools-extra/trunk/clang-tidy/performance/InefficientVectorOperationCheck.cpp
clang-tools-extra/trunk/clang-tidy/performance/InefficientVectorOperationCheck.h
clang-tools-extra/trunk/docs/clang-tidy/checks/performance-inefficient-vector-operation.rst
clang-tools-extra/trunk/test/clang-tidy/performance-inefficient-vector-operation.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
Modified: clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt?rev=300534&r1=300533&r2=300534&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt Tue Apr 18 02:46:39 2017
@@ -5,6 +5,7 @@ add_clang_library(clangTidyPerformanceMo
ForRangeCopyCheck.cpp
ImplicitCastInLoopCheck.cpp
InefficientStringConcatenationCheck.cpp
+ InefficientVectorOperationCheck.cpp
PerformanceTidyModule.cpp
TypePromotionInMathFnCheck.cpp
UnnecessaryCopyInitialization.cpp
Added: clang-tools-extra/trunk/clang-tidy/performance/InefficientVectorOperationCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/performance/InefficientVectorOperationCheck.cpp?rev=300534&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/performance/InefficientVectorOperationCheck.cpp (added)
+++ clang-tools-extra/trunk/clang-tidy/performance/InefficientVectorOperationCheck.cpp Tue Apr 18 02:46:39 2017
@@ -0,0 +1,149 @@
+//===--- InefficientVectorOperationCheck.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 "InefficientVectorOperationCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+#include "../utils/DeclRefExprUtils.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+namespace {
+
+// Matcher names. Given the code:
+//
+// \code
+// void f() {
+// vector<T> v;
+// for (int i = 0; i < 10 + 1; ++i) {
+// v.push_back(i);
+// }
+// }
+// \endcode
+//
+// The matcher names are bound to following parts of the AST:
+// - LoopName: The entire for loop (as ForStmt).
+// - LoopParentName: The body of function f (as CompoundStmt).
+// - VectorVarDeclName: 'v' in (as VarDecl).
+// - VectorVarDeclStmatName: The entire 'std::vector<T> v;' statement (as
+// DeclStmt).
+// - PushBackCallName: '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 LoopInitVarName[] = "loop_init_var";
+static const char LoopEndExprName[] = "loop_end_expr";
+
+} // namespace
+
+void InefficientVectorOperationCheck::registerMatchers(MatchFinder *Finder) {
+ const auto VectorDecl = cxxRecordDecl(hasName("::std::vector"));
+ const auto VectorDefaultConstructorCall = cxxConstructExpr(
+ hasType(VectorDecl),
+ hasDeclaration(cxxConstructorDecl(isDefaultConstructor())));
+ const auto VectorVarDecl =
+ varDecl(hasInitializer(VectorDefaultConstructorCall))
+ .bind(VectorVarDeclName);
+ const auto PushBackCallExpr =
+ cxxMemberCallExpr(
+ callee(cxxMethodDecl(hasName("push_back"))), on(hasType(VectorDecl)),
+ onImplicitObjectArgument(declRefExpr(to(VectorVarDecl))))
+ .bind(PushBackCallName);
+ const auto PushBackCall =
+ expr(anyOf(PushBackCallExpr, exprWithCleanups(has(PushBackCallExpr))));
+ const auto VectorVarDefStmt =
+ declStmt(hasSingleDecl(equalsBoundNode(VectorVarDeclName)))
+ .bind(VectorVarDeclStmtName);
+
+ const auto LoopVarInit =
+ declStmt(hasSingleDecl(varDecl(hasInitializer(integerLiteral(equals(0))))
+ .bind(LoopInitVarName)));
+ const auto RefersToLoopVar = ignoringParenImpCasts(
+ declRefExpr(to(varDecl(equalsBoundNode(LoopInitVarName)))));
+
+ // Match counter-based for loops:
+ // for (int i = 0; i < n; ++i) { v.push_back(...); }
+ //
+ // FIXME: Support more types of counter-based loops like decrement loops.
+ Finder->addMatcher(
+ forStmt(
+ hasLoopInit(LoopVarInit),
+ hasCondition(binaryOperator(
+ hasOperatorName("<"), hasLHS(RefersToLoopVar),
+ hasRHS(expr(unless(hasDescendant(expr(RefersToLoopVar))))
+ .bind(LoopEndExprName)))),
+ hasIncrement(unaryOperator(hasOperatorName("++"),
+ hasUnaryOperand(RefersToLoopVar))),
+ hasBody(anyOf(compoundStmt(statementCountIs(1), has(PushBackCall)),
+ PushBackCall)),
+ hasParent(compoundStmt(has(VectorVarDefStmt)).bind(LoopParentName)))
+ .bind(LoopCounterName),
+ this);
+}
+
+void InefficientVectorOperationCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ if (Result.Context->getDiagnostics().hasUncompilableErrorOccurred())
+ return;
+
+ const SourceManager &SM = *Result.SourceManager;
+ const auto *ForLoop = Result.Nodes.getNodeAs<ForStmt>(LoopCounterName);
+ const auto *PushBackCall =
+ Result.Nodes.getNodeAs<CXXMemberCallExpr>(PushBackCallName);
+ const auto *LoopEndExpr = Result.Nodes.getNodeAs<Expr>(LoopEndExprName);
+ const auto *LoopParent = Result.Nodes.getNodeAs<CompoundStmt>(LoopParentName);
+ const auto *VectorVarDecl =
+ Result.Nodes.getNodeAs<VarDecl>(VectorVarDeclName);
+
+ llvm::SmallPtrSet<const DeclRefExpr *, 16> AllVectorVarRefs =
+ utils::decl_ref_expr::allDeclRefExprs(*VectorVarDecl, *LoopParent,
+ *Result.Context);
+ for (const auto *Ref : AllVectorVarRefs) {
+ // Skip cases where there are usages (defined as DeclRefExpr that refers to
+ // "v") of vector variable `v` before the for loop. We consider these usages
+ // are operations causing memory preallocation (e.g. "v.resize(n)",
+ // "v.reserve(n)").
+ //
+ // FIXME: make it more intelligent to identify the pre-allocating operations
+ // before the for loop.
+ if (SM.isBeforeInTranslationUnit(Ref->getLocation(),
+ ForLoop->getLocStart())) {
+ return;
+ }
+ }
+
+ llvm::StringRef LoopEndSource = clang::Lexer::getSourceText(
+ CharSourceRange::getTokenRange(LoopEndExpr->getSourceRange()), SM,
+ clang::LangOptions());
+ llvm::StringRef VectorVarName = clang::Lexer::getSourceText(
+ CharSourceRange::getTokenRange(
+ PushBackCall->getImplicitObjectArgument()->getSourceRange()),
+ SM, clang::LangOptions());
+ std::string ReserveStmt =
+ (VectorVarName + ".reserve(" + LoopEndSource + ");\n").str();
+
+ diag(PushBackCall->getLocStart(),
+ "'push_back' is called inside a loop; "
+ "consider pre-allocating the vector capacity before the loop.")
+ << FixItHint::CreateInsertion(ForLoop->getLocStart(), ReserveStmt);
+}
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang
Added: clang-tools-extra/trunk/clang-tidy/performance/InefficientVectorOperationCheck.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/performance/InefficientVectorOperationCheck.h?rev=300534&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/performance/InefficientVectorOperationCheck.h (added)
+++ clang-tools-extra/trunk/clang-tidy/performance/InefficientVectorOperationCheck.h Tue Apr 18 02:46:39 2017
@@ -0,0 +1,36 @@
+//===--- InefficientVectorOperationCheck.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_PERFORMANCE_INEFFICIENT_VECTOR_OPERATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENT_VECTOR_OPERATION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+/// Finds possible inefficient `std::vector` operations (e.g. `push_back`) in
+/// for loops that may cause unnecessary memory reallocations.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/performance-inefficient-vector-operation.html
+class InefficientVectorOperationCheck : public ClangTidyCheck {
+public:
+ InefficientVectorOperationCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENT_VECTOR_OPERATION_H
Modified: clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp?rev=300534&r1=300533&r2=300534&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp Tue Apr 18 02:46:39 2017
@@ -14,6 +14,7 @@
#include "ForRangeCopyCheck.h"
#include "ImplicitCastInLoopCheck.h"
#include "InefficientStringConcatenationCheck.h"
+#include "InefficientVectorOperationCheck.h"
#include "TypePromotionInMathFnCheck.h"
#include "UnnecessaryCopyInitialization.h"
#include "UnnecessaryValueParamCheck.h"
@@ -33,6 +34,8 @@ public:
"performance-implicit-cast-in-loop");
CheckFactories.registerCheck<InefficientStringConcatenationCheck>(
"performance-inefficient-string-concatenation");
+ CheckFactories.registerCheck<InefficientVectorOperationCheck>(
+ "performance-inefficient-vector-operation");
CheckFactories.registerCheck<TypePromotionInMathFnCheck>(
"performance-type-promotion-in-math-fn");
CheckFactories.registerCheck<UnnecessaryCopyInitialization>(
Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=300534&r1=300533&r2=300534&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Tue Apr 18 02:46:39 2017
@@ -62,7 +62,7 @@ Improvements to clang-tidy
Finds modification of the ``std`` or ``posix`` namespace.
-- Improved `cppcoreguidelines-no-malloc
+- Improved `cppcoreguidelines-no-malloc
<http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-no-malloc.html>`_ check
Allow custom memory management functions to be considered as well.
@@ -95,6 +95,12 @@ Improvements to clang-tidy
- Support clang-formatting of the code around applied fixes (``-format-style``
command-line option).
+- New `performance-inefficient-vector-operation
+ <http://clang.llvm.org/extra/clang-tidy/checks/performance-inefficient-vector-operation.html>`_ check
+
+ Finds possible inefficient vector operations in for loops that may cause
+ unnecessary memory reallocations.
+
Improvements to include-fixer
-----------------------------
Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst?rev=300534&r1=300533&r2=300534&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst Tue Apr 18 02:46:39 2017
@@ -142,6 +142,7 @@ Clang-Tidy Checks
performance-for-range-copy
performance-implicit-cast-in-loop
performance-inefficient-string-concatenation
+ performance-inefficient-vector-operation
performance-type-promotion-in-math-fn
performance-unnecessary-copy-initialization
performance-unnecessary-value-param
Added: clang-tools-extra/trunk/docs/clang-tidy/checks/performance-inefficient-vector-operation.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/performance-inefficient-vector-operation.rst?rev=300534&view=auto
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/performance-inefficient-vector-operation.rst (added)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/performance-inefficient-vector-operation.rst Tue Apr 18 02:46:39 2017
@@ -0,0 +1,20 @@
+.. title:: clang-tidy - performance-inefficient-vector-operation
+
+performance-inefficient-vector-operation
+========================================
+
+Finds possible inefficient `std::vector` operations (e.g. `push_back`) that may
+cause unnecessary memory reallocations.
+
+Currently, the check only detects a typical counter-based for loop with a single
+statement in it, see below:
+
+.. code-block:: c++
+
+ std::vector<int> v;
+ for (int i = 0; i < n; ++i) {
+ v.push_back(n);
+ // This will trigger the warning since the push_back may cause multiple
+ // memory reallocations in v. This can be avoid by inserting a 'reserve(n)'
+ // statment before the for statment.
+ }
Added: clang-tools-extra/trunk/test/clang-tidy/performance-inefficient-vector-operation.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/performance-inefficient-vector-operation.cpp?rev=300534&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/performance-inefficient-vector-operation.cpp (added)
+++ clang-tools-extra/trunk/test/clang-tidy/performance-inefficient-vector-operation.cpp Tue Apr 18 02:46:39 2017
@@ -0,0 +1,183 @@
+// RUN: %check_clang_tidy %s performance-inefficient-vector-operation %t -- -format-style=llvm --
+
+typedef int size_t;
+
+namespace std {
+template <class T>
+class vector {
+ public:
+ typedef T* iterator;
+ typedef const T* const_iterator;
+ typedef T& reference;
+ typedef const T& const_reference;
+ typedef size_t size_type;
+
+ explicit vector();
+ explicit vector(size_type n);
+
+ void push_back(const T& val);
+ void reserve(size_t n);
+ void resize(size_t n);
+
+ size_t size();
+ const_reference operator[] (size_type) const;
+ reference operator[] (size_type);
+};
+} // namespace std
+
+void f(std::vector<int>& t) {
+ {
+ std::vector<int> v;
+ // CHECK-FIXES: v.reserve(10);
+ for (int i = 0; i < 10; ++i)
+ v.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);
+ for (int i = 0; i < 10; i++)
+ v.push_back(i);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
+ }
+ {
+ std::vector<int> v;
+ // CHECK-FIXES: v.reserve(10);
+ for (int i = 0; i < 10; ++i)
+ v.push_back(0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
+ }
+ {
+ std::vector<int> v;
+ // CHECK-FIXES: v.reserve(5);
+ for (int i = 0; i < 5; ++i) {
+ v.push_back(i);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
+ }
+ // CHECK-FIXES-NOT: v.reserve(10);
+ for (int i = 0; i < 10; ++i) {
+ // No fix for this loop as we encounter the prior loops.
+ v.push_back(i);
+ }
+ }
+ {
+ std::vector<int> v;
+ std::vector<int> v2;
+ v2.reserve(3);
+ // CHECK-FIXES: v.reserve(10);
+ for (int i = 0; i < 10; ++i)
+ v.push_back(i);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
+ }
+ {
+ std::vector<int> v;
+ // CHECK-FIXES: v.reserve(t.size());
+ for (size_t i = 0; i < t.size(); ++i) {
+ v.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);
+ for (size_t i = 0; i < t.size() - 1; ++i) {
+ v.push_back(t[i]);
+ } // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
+ }
+
+ // ---- Non-fixed Cases ----
+ {
+ std::vector<int> v;
+ v.reserve(20);
+ // CHECK-FIXES-NOT: v.reserve(10);
+ // There is a "reserve" call already.
+ for (int i = 0; i < 10; ++i) {
+ v.push_back(i);
+ }
+ }
+ {
+ std::vector<int> v;
+ v.reserve(5);
+ // CHECK-FIXES-NOT: v.reserve(10);
+ // There is a "reserve" call already.
+ for (int i = 0; i < 10; ++i) {
+ v.push_back(i);
+ }
+ }
+ {
+ std::vector<int> v;
+ v.resize(5);
+ // CHECK-FIXES-NOT: v.reserve(10);
+ // There is a ref usage of v before the loop.
+ for (int i = 0; i < 10; ++i) {
+ v.push_back(i);
+ }
+ }
+ {
+ std::vector<int> v;
+ v.push_back(0);
+ // CHECK-FIXES-NOT: v.reserve(10);
+ // There is a ref usage of v before the loop.
+ for (int i = 0; i < 10; ++i) {
+ v.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.
+ for (int i = 0; i < 10; ++i) {
+ v.push_back(i);
+ }
+ }
+ {
+ std::vector<int> v(20);
+ // CHECK-FIXES-NOT: v.reserve(10);
+ // v is not constructed with default constructor.
+ for (int i = 0; i < 10; ++i) {
+ v.push_back(i);
+ }
+ }
+ {
+ std::vector<int> v;
+ // CHECK-FIXES-NOT: v.reserve(10);
+ // For-loop is not started with 0.
+ for (int i = 1; i < 10; ++i) {
+ v.push_back(i);
+ }
+ }
+ {
+ std::vector<int> v;
+ // CHECK-FIXES-NOT: v.reserve(t.size());
+ // v isn't referenced in for-loop body.
+ for (size_t i = 0; i < t.size(); ++i) {
+ t.push_back(i);
+ }
+ }
+ {
+ std::vector<int> v;
+ int k;
+ // CHECK-FIXES-NOT: v.reserve(10);
+ // For-loop isn't a fixable loop.
+ for (size_t i = 0; k < 10; ++i) {
+ v.push_back(t[i]);
+ }
+ }
+ {
+ std::vector<int> v;
+ // CHECK-FIXES-NOT: v.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);
+ }
+ {
+ std::vector<int> v;
+ int k;
+ // CHECK-FIXES-NOT: v.reserve(10);
+ // For-loop isn't a fixable loop.
+ for (size_t i = 0; i < 10; ++k) {
+ v.push_back(t[i]);
+ }
+ }
+}
More information about the cfe-commits
mailing list