njames93 updated this revision to Diff 245240.
njames93 added a comment.
- Better template support
- Removed excess code
- Refactor alot of the check
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74689/new/
https://reviews.llvm.org/D74689
Files:
clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp
clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/performance-inefficient-vector-operation.rst
clang-tools-extra/test/clang-tidy/checkers/performance-inefficient-vector-operation.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/performance-inefficient-vector-operation.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/performance-inefficient-vector-operation.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-inefficient-vector-operation.cpp
@@ -1,7 +1,11 @@
// RUN: %check_clang_tidy %s performance-inefficient-vector-operation %t -- \
-// RUN: -format-style=llvm \
// RUN: -config='{CheckOptions: \
-// RUN: [{key: performance-inefficient-vector-operation.EnableProto, value: 1}]}'
+// RUN: [{key: performance-inefficient-vector-operation.EnableProto, value: 1}, \
+// RUN: {key: performance-inefficient-vector-operation.VectorLikeClasses, value : MyContainer}, \
+// RUN: {key: performance-inefficient-vector-operation.SupportedRanges, value : MyContainer}, \
+// RUN: {key: performance-inefficient-vector-operation.ReserveNames, value : Reserve}, \
+// RUN: {key: performance-inefficient-vector-operation.AppendNames, value : PushBack}, \
+// RUN: {key: performance-inefficient-vector-operation.SizeNames, value : Size}, ]}'
namespace std {
@@ -359,3 +363,273 @@
}
}
}
+
+namespace OptionsValidMatchDefault {
+template <typename T>
+class MyContainer {
+public:
+ unsigned size() const;
+ T *begin() const;
+ T *end() const;
+ void push_back(const T &);
+ void reserve(unsigned);
+};
+
+void foo(const MyContainer<int> &C) {
+ MyContainer<int> CC1;
+ // CHECK-FIXES: {{^}} CC1.reserve(C.size());
+ for (auto I : C) {
+ CC1.push_back(I);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'push_back' is called
+ }
+}
+} // namespace OptionsValidMatchDefault
+
+namespace OptionsValidMatchDifferentMethods {
+template <typename T>
+class MyContainer {
+public:
+ unsigned Size() const;
+ T *begin() const;
+ T *end() const;
+ void PushBack(const T &);
+ void Reserve(unsigned);
+};
+
+void foo(const MyContainer<int> &C) {
+ MyContainer<int> CC2;
+ // CHECK-FIXES: {{^}} CC2.Reserve(C.Size());
+ for (auto I : C) {
+ CC2.PushBack(I);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'PushBack' is called
+ }
+}
+} // namespace OptionsValidMatchDifferentMethods
+
+namespace UnknownContainer {
+template <typename T>
+class MyUContainer {
+public:
+ unsigned size() const;
+ T *begin() const;
+ T *end() const;
+ void push_back(const T &);
+ void reserve(unsigned);
+};
+
+void foo(const MyUContainer<int> &C) {
+ // MyUContainer isn't specified as a VectorLikeClass in the Config Options
+ MyUContainer<int> CC3;
+ // CHECK-FIXES-NOT: {{^}} CC3.reserve(C.size());
+ for (auto I : C) {
+ CC3.push_back(I);
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: 'push_back' is called
+ }
+}
+} // namespace UnknownContainer
+
+namespace PrivateMethods {
+namespace Size {
+template <typename T>
+class MyContainer {
+ unsigned size() const;
+
+public:
+ T *begin() const;
+ T *end() const;
+ void push_back(const T &);
+ void reserve(unsigned);
+};
+
+void foo(const MyContainer<int> &C) {
+ // MyContainer<int>::size is private, so calling it will be invalid
+ MyContainer<int> CC4;
+ // CHECK-FIXES-NOT: {{^}} CC4.reserve(C.size());
+ for (auto I : C) {
+ CC4.push_back(I);
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: 'push_back' is called
+ }
+}
+} // namespace Size
+namespace Reserve {
+template <typename T>
+class MyContainer {
+public:
+ unsigned size() const;
+ T *begin() const;
+ T *end() const;
+ void push_back(const T &);
+
+private:
+ void reserve(unsigned);
+};
+
+void foo(const MyContainer<int> &C) {
+ // MyContainer<int>::reserve is private, so calling it will be invalid
+ MyContainer<int> CC5;
+ // CHECK-FIXES-NOT: {{^}} CC5.reserve(C.size());
+ for (auto I : C) {
+ CC5.push_back(I);
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: 'push_back' is called
+ }
+}
+} // namespace Reserve
+} // namespace PrivateMethods
+
+namespace BadSignatures {
+namespace Size {
+template <typename T>
+class MyContainer {
+public:
+ char *size() const;
+ T *begin() const;
+ T *end() const;
+ void push_back(const T &);
+ void reserve(unsigned);
+};
+
+void foo(const MyContainer<int> &C) {
+ // MyContainer<int>::size doesn't return an integral type(char *), so ignore this class
+ MyContainer<int> CC6;
+ // CHECK-FIXES-NOT: {{^}} CC6.reserve(C.size());
+ for (auto I : C) {
+ CC6.push_back(I);
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: 'push_back' is called
+ }
+}
+} // namespace Size
+namespace Reserve {
+template <typename T>
+class MyContainer {
+public:
+ unsigned size() const;
+ T *begin() const;
+ T *end() const;
+ void push_back(const T &);
+ void reserve();
+};
+
+void foo(const MyContainer<int> &C) {
+ // MyContainer<int>::reserve doesn't take a single integral argument, so ignore this class
+ MyContainer<int> CC7;
+ // CHECK-FIXES-NOT: {{^}} CC7.reserve(C.size());
+ for (auto I : C) {
+ CC7.push_back(I);
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: 'push_back' is called
+ }
+}
+} // namespace Reserve
+} // namespace BadSignatures
+
+namespace NonConstSizeMethod {
+template <typename T>
+class MyContainer {
+public:
+ unsigned size();
+ T *begin() const;
+ T *end() const;
+ void push_back(const T &);
+ void reserve(unsigned);
+};
+
+void foo(const MyContainer<int> &C) {
+ // MyContainer<int>::size() isn't const, therefore we cant call C.size() as
+ // C is passed by const ref.
+ MyContainer<int> CC8;
+ // CHECK-FIXES-NOT: {{^}} CC8.reserve(C.size());
+ for (auto I : C) {
+ CC8.push_back(I);
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: 'push_back' is called
+ }
+}
+
+void bar(MyContainer<int> &C) {
+ // MyContainer<int>::size() isn't const, but C is passed by non-const ref so
+ // we can still call it.
+ MyContainer<int> CC9;
+ // CHECK-FIXES: {{^}} CC9.reserve(C.size());
+ for (auto I : C) {
+ CC9.push_back(I);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'push_back' is called
+ }
+}
+} // namespace NonConstSizeMethod
+
+namespace ArrayForRange {
+int getSize();
+void foo() {
+ {
+ std::vector<int> A1;
+ int Arr[] = {1, 2, 3};
+ // CHECK-FIXES: {{^}} A1.reserve(3);
+ for (auto X : Arr) {
+ A1.push_back(X);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
+ }
+ }
+ {
+ std::vector<int> A2;
+ int Arr[3];
+ // CHECK-FIXES: {{^}} A2.reserve(3);
+ for (auto X : Arr) {
+ A2.push_back(X);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
+ }
+ }
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wvla-extension"
+ {
+ std::vector<int> A3;
+ int Size = getSize();
+ int Arr[Size];
+ // CHECK-FIXES: {{^}} A3.reserve(Size);
+ for (auto X : Arr) {
+ A3.push_back(X);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
+ }
+ }
+ {
+ std::vector<int> A4;
+ int Arr[getSize()];
+ // CHECK-FIXES: {{^}} A4.reserve(sizeof(Arr) / sizeof(Arr[0]));
+ for (auto X : Arr) {
+ A4.push_back(X);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'push_back' is called
+ }
+ }
+#pragma clang diagnostic pop
+}
+
+template <unsigned N>
+constexpr unsigned Times2 = 2 * N;
+
+template <unsigned N>
+void foo(const int (&Param)[N]) {
+ std::vector<int> A5;
+ int Arr[N];
+ // CHECK-FIXES: {{^}} A5.reserve(N);
+ for (auto X : Arr) {
+ A5.push_back(X);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'push_back' is called
+ }
+ std::vector<int> A6;
+ int CompArr[Times2<N>];
+ // CHECK-FIXES: {{^}} A6.reserve(Times2<N>);
+ for (auto X : CompArr) {
+ A6.emplace_back(X);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'emplace_back' is called
+ }
+ std::vector<int> A7;
+ // CHECK-FIXES: {{^}} A7.reserve(N);
+ for (auto X : Param) {
+ A7.push_back(X);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'push_back' is called
+ }
+}
+
+void b() {
+ int Arr[4];
+ foo(Arr); // Ensure this doesn't try to insert reserve statements based on
+ // the instantiation value of 4
+}
+} // namespace ArrayForRange
\ No newline at end of file
Index: clang-tools-extra/docs/clang-tidy/checks/performance-inefficient-vector-operation.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/performance-inefficient-vector-operation.rst
+++ clang-tools-extra/docs/clang-tidy/checks/performance-inefficient-vector-operation.rst
@@ -34,9 +34,7 @@
}
* For-range loops like ``for (range-declaration : range_expression)``, the type
- of ``range_expression`` can be ``std::vector``, ``std::array``,
- ``std::deque``, ``std::set``, ``std::unordered_set``, ``std::map``,
- ``std::unordered_set``:
+ of ``range_expression`` are specified in :option:`SupportedRanges`:
.. code-block:: c++
@@ -59,6 +57,32 @@
Semicolon-separated list of names of vector-like classes. By default only
``::std::vector`` is considered.
+.. option:: AppendNames
+
+ Semicolon-separated list of names of append methods in :option:`VectorLikeClasses`.
+ By default only ``push_back`` and ``emplace_back`` are considered.
+
+.. option:: ReserveNames
+
+ Semicolon-separated list of names of reserve methods in :option:`VectorLikeClasses`.
+ By default only ``reserve`` is considered.
+
+ Note the method must have `1` parameter that is of integral type.
+
+.. option:: SupportedRanges
+
+ Semicolon-separated list of names of Range types for for-range expressions.
+ By default only ``std::vector``, ``std::array``, ``std::deque``,
+ ``std::set``, ``std::unordered_set``, ``std::map`` and ``std::unordered_set``
+ are considered.
+
+.. option:: SizeNames
+
+ Semicolon-separated list of names of size methods :option:`SupportedRanges`.
+ By default only ``size`` is considered.
+
+ Note the method must have `0` parameters and return an integral type.
+
.. option:: EnableProto
When non-zero, the check will also warn on inefficient operations for proto
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -114,6 +114,10 @@
Changes in existing checks
^^^^^^^^^^^^^^^^^^^^^^^^^^
+- Improved :doc:`performance-inefficient-vector-operation
+ <clang-tidy/checks/performance-inefficient-vector-operation>` check now has
+ extra options for custom containers.
+
- Improved :doc:`readability-qualified-auto
<clang-tidy/checks/readability-qualified-auto>` check now supports a
`AddConstToQualified` to enable adding ``const`` qualifiers to variables
Index: clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.h
+++ clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.h
@@ -31,11 +31,15 @@
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
private:
- void AddMatcher(const ast_matchers::DeclarationMatcher &TargetRecordDecl,
+ void addMatcher(const ast_matchers::DeclarationMatcher &TargetRecordDecl,
StringRef VarDeclName, StringRef VarDeclStmtName,
const ast_matchers::DeclarationMatcher &AppendMethodDecl,
StringRef AppendCallName, ast_matchers::MatchFinder *Finder);
- const std::vector<std::string> VectorLikeClasses;
+ const std::string VectorLikeClasses;
+ const std::string AppendNames;
+ const std::string ReserveNames;
+ const std::string SupportedRanges;
+ const std::string SizeNames;
// If true, also check inefficient operations for proto repeated fields.
bool EnableProto;
Index: clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp
@@ -7,11 +7,13 @@
//===----------------------------------------------------------------------===//
#include "InefficientVectorOperationCheck.h"
+
+#include "../utils/DeclRefExprUtils.h"
+#include "../utils/OptionsUtils.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Lex/Lexer.h"
-#include "../utils/DeclRefExprUtils.h"
-#include "../utils/OptionsUtils.h"
+#include "llvm/Support/raw_ostream.h"
using namespace clang::ast_matchers;
@@ -38,53 +40,114 @@
// \endcode
//
// The matcher names are bound to following parts of the AST:
-// - LoopCounterName: The entire for loop (as ForStmt).
// - LoopParentName: The body of function f (as CompoundStmt).
-// - VectorVarDeclName: 'v' (as VarDecl).
-// - VectorVarDeclStmatName: The entire 'std::vector<T> v;' statement (as
-// DeclStmt).
-// - PushBackOrEmplaceBackCallName: 'v.push_back(i)' (as cxxMemberCallExpr).
+// - VarDeclName: 'v' (as VarDecl).
+// - VarDeclStmtName: The entire 'std::vector<T> v;' or
+// 'SomeProto p;' statement (as DeclStmt).
+// - AppendMethodName: 'v.push_back(i), p.add_xxx(i)' (as CallExpr).
+// - VectorReserveName: The name of the reserve method in the VectorClass
+// e.g `reserve(size_type amountToReserve);`. If this isn't matched then we
+// are dealing with a Proto.
+// If A ForLoop is matched, the related names are bound to the following parts:
+// - LoopCounterName: The entire for loop (as ForStmt).
// - LoopInitVarName: 'i' (as VarDecl).
-// - LoopEndExpr: '10+1' (as Expr).
-// If EnableProto, the proto related names are bound to the following parts:
-// - ProtoVarDeclName: 'p' (as VarDecl).
-// - ProtoVarDeclStmtName: The entire 'SomeProto p;' statement (as DeclStmt).
-// - ProtoAddFieldCallName: 'p.add_xxx(i)' (as cxxMemberCallExpr).
-static const char LoopCounterName[] = "for_loop_counter";
+// - LoopEndExprName: '10+1' (as Expr).
+// If A RangeFor is matched, the related named are bound to the following parts:
+// - RangeLoopName: The entire for range loop (as CXXForRangeStmt).
+// - RangeLoopSizeMethodName: The name of the size method in the VectorClass
+// e.g. `size_type size() const;`. If this isn't matched then we are dealing
+// with a range over a c style array.
static const char LoopParentName[] = "loop_parent";
-static const char VectorVarDeclName[] = "vector_var_decl";
-static const char VectorVarDeclStmtName[] = "vector_var_decl_stmt";
-static const char PushBackOrEmplaceBackCallName[] = "append_call";
-static const char ProtoVarDeclName[] = "proto_var_decl";
-static const char ProtoVarDeclStmtName[] = "proto_var_decl_stmt";
-static const char ProtoAddFieldCallName[] = "proto_add_field";
+static const char VarDeclName[] = "var_decl";
+static const char VarDeclStmtName[] = "var_decl_stmt";
+static const char AppendMethodName[] = "append_call";
+static const char VectorReserveName[] = "vector_reserve_name";
+static const char LoopCounterName[] = "for_loop_counter";
static const char LoopInitVarName[] = "loop_init_var";
static const char LoopEndExprName[] = "loop_end_expr";
static const char RangeLoopName[] = "for_range_loop";
+static const char RangeLoopSizeMethodName[] = "for_range_size_name";
+static const char SupportedDefaultRanges[] = "::std::vector;"
+ "::std::set;"
+ "::std::unordered_set;"
+ "::std::map;"
+ "::std::unordered_map;"
+ "::std::array;"
+ "::std::deque";
-ast_matchers::internal::Matcher<Expr> supportedContainerTypesMatcher() {
- return hasType(cxxRecordDecl(hasAnyName(
- "::std::vector", "::std::set", "::std::unordered_set", "::std::map",
- "::std::unordered_map", "::std::array", "::std::deque")));
+static ast_matchers::internal::Matcher<NamedDecl>
+hasAnyNameStdString(std::vector<std::string> Names) {
+ return ast_matchers::internal::Matcher<NamedDecl>(
+ new ast_matchers::internal::HasNameMatcher(std::move(Names)));
}
+AST_MATCHER_P(FunctionTemplateDecl, hasTemplatedDecl,
+ ast_matchers::internal::Matcher<FunctionDecl>, InnerMatcher) {
+ FunctionDecl *Decl = Node.getTemplatedDecl();
+ return Decl && InnerMatcher.matches(*Decl, Finder, Builder);
+}
+
+static std::vector<std::string> parseStringListPair(StringRef LHS,
+ StringRef RHS) {
+ if (LHS.empty()) {
+ if (RHS.empty())
+ return {};
+ return utils::options::parseStringList(RHS);
+ }
+ if (RHS.empty())
+ return utils::options::parseStringList(LHS);
+ llvm::SmallString<512> Buffer;
+ return utils::options::parseStringList((LHS + ";" + RHS).toStringRef(Buffer));
+}
+
+static ast_matchers::internal::Matcher<NamedDecl>
+hasAnyNameListPair(StringRef LHS, StringRef RHS) {
+ return hasAnyNameStdString(parseStringListPair(LHS, RHS));
+}
+struct MemberCallData {
+ const Expr *Base;
+ DeclarationName Name;
+ SourceLocation Location;
+};
+
+static llvm::Optional<MemberCallData> getMemberCallData(const CallExpr *Call) {
+ assert(Call && "no append call expression");
+ if (const auto *Cxx = dyn_cast<CXXMemberCallExpr>(Call))
+ return MemberCallData{Cxx->getImplicitObjectArgument(),
+ Cxx->getMethodDecl()->getDeclName(),
+ Call->getBeginLoc()};
+ const auto *Callee = Call->getCallee();
+ if (const auto *Mem = dyn_cast<MemberExpr>(Callee))
+ return MemberCallData{Mem->getBase(), Mem->getMemberDecl()->getDeclName(),
+ Call->getBeginLoc()};
+ if (const auto *Unres = dyn_cast<UnresolvedMemberExpr>(Callee))
+ return MemberCallData{Unres->getBase(), Unres->getName(),
+ Call->getBeginLoc()};
+ return None;
+}
} // namespace
InefficientVectorOperationCheck::InefficientVectorOperationCheck(
StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
- VectorLikeClasses(utils::options::parseStringList(
- Options.get("VectorLikeClasses", "::std::vector"))),
+ VectorLikeClasses(Options.get("VectorLikeClasses", "")),
+ AppendNames(Options.get("AppendNames", "")),
+ ReserveNames(Options.get("ReserveNames", "")),
+ SupportedRanges(Options.get("SupportedRanges", "")),
+ SizeNames(Options.get("SizeNames", "")),
EnableProto(Options.getLocalOrGlobal("EnableProto", false)) {}
void InefficientVectorOperationCheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {
- Options.store(Opts, "VectorLikeClasses",
- utils::options::serializeStringList(VectorLikeClasses));
+ Options.store(Opts, "VectorLikeClasses", VectorLikeClasses);
+ Options.store(Opts, "AppendNames", AppendNames);
+ Options.store(Opts, "ReserveNames", ReserveNames);
+ Options.store(Opts, "SupportedRanges", SupportedRanges);
+ Options.store(Opts, "SizeNames", SizeNames);
Options.store(Opts, "EnableProto", EnableProto);
}
-void InefficientVectorOperationCheck::AddMatcher(
+void InefficientVectorOperationCheck::addMatcher(
const DeclarationMatcher &TargetRecordDecl, StringRef VarDeclName,
StringRef VarDeclStmtName, const DeclarationMatcher &AppendMethodDecl,
StringRef AppendCallName, MatchFinder *Finder) {
@@ -97,12 +160,33 @@
declStmt(hasSingleDecl(equalsBoundNode(std::string(VarDeclName))))
.bind(VarDeclStmtName);
+ // this handles unresolved functions that are templates(e.g. emplace_back)
+ const auto TemplateResolved =
+ decl(anyOf(AppendMethodDecl,
+ functionTemplateDecl(hasTemplatedDecl(AppendMethodDecl))));
+
const auto AppendCallExpr =
cxxMemberCallExpr(
- callee(AppendMethodDecl), on(hasType(TargetRecordDecl)),
+ callee(TemplateResolved), on(hasType(TargetRecordDecl)),
onImplicitObjectArgument(declRefExpr(to(TargetVarDecl))))
.bind(AppendCallName);
- const auto AppendCall = expr(ignoringImplicit(AppendCallExpr));
+
+ // In templates sometimes the call expr can't be resolved to a
+ // CXXMemberCallExpr so handle these edge cases
+ const auto MemberCallExpr =
+ callExpr(callee(memberExpr(
+ hasDeclaration(TemplateResolved),
+ hasObjectExpression(hasType(TargetRecordDecl)),
+ hasObjectExpression(declRefExpr(to(TargetVarDecl))))))
+ .bind(AppendCallName);
+ const auto UnresolvedCallExpr =
+ callExpr(callee(unresolvedMemberExpr(
+ hasAnyDeclaration(TemplateResolved),
+ hasObjectExpression(hasType(TargetRecordDecl)),
+ hasObjectExpression(declRefExpr(to(TargetVarDecl))))))
+ .bind(AppendCallName);
+ const auto AppendCall = expr(ignoringImplicit(
+ anyOf(AppendCallExpr, MemberCallExpr, UnresolvedCallExpr)));
const auto LoopVarInit =
declStmt(hasSingleDecl(varDecl(hasInitializer(integerLiteral(equals(0))))
.bind(LoopInitVarName)));
@@ -136,6 +220,10 @@
.bind(LoopCounterName),
this);
+ const auto SupportedRangeName =
+ hasAnyNameListPair(SupportedDefaultRanges, SupportedRanges);
+ const auto SupportedSizeName = hasAnyNameListPair("size", SizeNames);
+
// Match for-range loops:
// for (const auto& E : data) {
// v.push_back(...);
@@ -143,21 +231,48 @@
// }
//
// FIXME: Support more complex range-expressions.
+ auto RangeMatcher = [&](bool CheckConst) {
+ return cxxForRangeStmt(
+ hasRangeInit(declRefExpr(
+ (CheckConst ? expr(hasType(isConstQualified()))
+ : expr(unless(hasType(isConstQualified())))),
+ hasType(cxxRecordDecl(
+ SupportedRangeName,
+ hasMethod(
+ cxxMethodDecl(SupportedSizeName, parameterCountIs(0),
+ isPublic(),
+ (CheckConst ? isConst() : anything()),
+ returns(isInteger()))
+ .bind(RangeLoopSizeMethodName)))))),
+ HasInterestingLoopBody, InInterestingCompoundStmt)
+ .bind(RangeLoopName);
+ };
+ Finder->addMatcher(RangeMatcher(true), this);
+ Finder->addMatcher(RangeMatcher(false), this);
Finder->addMatcher(
cxxForRangeStmt(
- hasRangeInit(declRefExpr(supportedContainerTypesMatcher())),
- HasInterestingLoopBody, InInterestingCompoundStmt)
+ hasRangeInit(declRefExpr(hasType(ignoringParens(arrayType())))),
+ unless(isInTemplateInstantiation()), HasInterestingLoopBody,
+ InInterestingCompoundStmt)
.bind(RangeLoopName),
this);
}
void InefficientVectorOperationCheck::registerMatchers(MatchFinder *Finder) {
- const auto VectorDecl = cxxRecordDecl(hasAnyName(SmallVector<StringRef, 5>(
- VectorLikeClasses.begin(), VectorLikeClasses.end())));
+
+ if (!getLangOpts().CPlusPlus)
+ return;
+
+ const auto VectorDecl = cxxRecordDecl(
+ hasAnyNameListPair("::std::vector", VectorLikeClasses),
+ hasMethod(cxxMethodDecl(hasAnyNameListPair("reserve", ReserveNames),
+ isPublic(), parameterCountIs(1),
+ hasParameter(0, hasType(isInteger())))
+ .bind(VectorReserveName)));
const auto AppendMethodDecl =
- cxxMethodDecl(hasAnyName("push_back", "emplace_back"));
- AddMatcher(VectorDecl, VectorVarDeclName, VectorVarDeclStmtName,
- AppendMethodDecl, PushBackOrEmplaceBackCallName, Finder);
+ cxxMethodDecl(hasAnyNameListPair("push_back;emplace_back", AppendNames));
+ addMatcher(VectorDecl, VarDeclName, VarDeclStmtName, AppendMethodDecl,
+ AppendMethodName, Finder);
if (EnableProto) {
const auto ProtoDecl =
@@ -168,8 +283,8 @@
// with "add_". So we exclude const methods.
const auto AddFieldMethodDecl =
cxxMethodDecl(matchesName("::add_"), unless(isConst()));
- AddMatcher(ProtoDecl, ProtoVarDeclName, ProtoVarDeclStmtName,
- AddFieldMethodDecl, ProtoAddFieldCallName, Finder);
+ addMatcher(ProtoDecl, VarDeclName, VarDeclStmtName, AddFieldMethodDecl,
+ AppendMethodName, Finder);
}
}
@@ -180,34 +295,28 @@
return;
const SourceManager &SM = *Result.SourceManager;
- const auto *VectorVarDecl =
- Result.Nodes.getNodeAs<VarDecl>(VectorVarDeclName);
+ auto GetSourceText = [&](const Stmt *Statement) {
+ return Lexer::getSourceText(
+ CharSourceRange::getTokenRange(Statement->getSourceRange()), SM,
+ Context->getLangOpts());
+ };
+ const auto *VDecl = Result.Nodes.getNodeAs<VarDecl>(VarDeclName);
const auto *ForLoop = Result.Nodes.getNodeAs<ForStmt>(LoopCounterName);
const auto *RangeLoop =
Result.Nodes.getNodeAs<CXXForRangeStmt>(RangeLoopName);
- const auto *VectorAppendCall =
- Result.Nodes.getNodeAs<CXXMemberCallExpr>(PushBackOrEmplaceBackCallName);
- const auto *ProtoVarDecl = Result.Nodes.getNodeAs<VarDecl>(ProtoVarDeclName);
- const auto *ProtoAddFieldCall =
- Result.Nodes.getNodeAs<CXXMemberCallExpr>(ProtoAddFieldCallName);
- const auto *LoopEndExpr = Result.Nodes.getNodeAs<Expr>(LoopEndExprName);
const auto *LoopParent = Result.Nodes.getNodeAs<CompoundStmt>(LoopParentName);
- const CXXMemberCallExpr *AppendCall =
- VectorAppendCall ? VectorAppendCall : ProtoAddFieldCall;
- assert(AppendCall && "no append call expression");
+ auto AppendCallInfo =
+ getMemberCallData(Result.Nodes.getNodeAs<CallExpr>(AppendMethodName));
- const Stmt *LoopStmt = ForLoop;
- if (!LoopStmt)
- LoopStmt = RangeLoop;
+ if (!AppendCallInfo)
+ return;
- const auto *TargetVarDecl = VectorVarDecl;
- if (!TargetVarDecl)
- TargetVarDecl = ProtoVarDecl;
+ const Stmt *LoopStmt = ForLoop ? cast<Stmt>(ForLoop) : RangeLoop;
+ assert(LoopStmt && "LoopStmt shouldn't be null on a match");
llvm::SmallPtrSet<const DeclRefExpr *, 16> AllVarRefs =
- utils::decl_ref_expr::allDeclRefExprs(*TargetVarDecl, *LoopParent,
- *Context);
+ utils::decl_ref_expr::allDeclRefExprs(*VDecl, *LoopParent, *Context);
for (const auto *Ref : AllVarRefs) {
// Skip cases where there are usages (defined as DeclRefExpr that refers
// to "v") of vector variable / proto variable `v` before the for loop. We
@@ -222,49 +331,72 @@
}
}
- std::string PartialReserveStmt;
- if (VectorAppendCall != nullptr) {
- PartialReserveStmt = ".reserve";
+ SmallString<64> Buffer;
+ llvm::raw_svector_ostream FixIt(Buffer);
+
+ // Add the name to the FixIt insertion
+ FixIt << GetSourceText(AppendCallInfo->Base);
+
+ // Add the start of the reserve to the FixIt insertion
+ // <Name>.reserve(
+ if (const auto *ReserveMethod =
+ Result.Nodes.getNodeAs<CXXMethodDecl>(VectorReserveName)) {
+ FixIt << '.' << ReserveMethod->getName() << '(';
} else {
- llvm::StringRef FieldName = ProtoAddFieldCall->getMethodDecl()->getName();
+ // No found reserve method, must be a Proto method
+ llvm::StringRef FieldName =
+ AppendCallInfo->Name.getAsIdentifierInfo()->getName();
+ assert(FieldName.startswith("add_") &&
+ "Method call didn't start with 'add_'");
FieldName.consume_front("add_");
- std::string MutableFieldName = ("mutable_" + FieldName).str();
- PartialReserveStmt = "." + MutableFieldName +
- "()->Reserve"; // e.g., ".mutable_xxx()->Reserve"
+ FixIt << ".mutable_" << FieldName
+ << "()->Reserve("; // e.g., ".mutable_xxx()->Reserve("
}
- llvm::StringRef VarName = Lexer::getSourceText(
- CharSourceRange::getTokenRange(
- AppendCall->getImplicitObjectArgument()->getSourceRange()),
- SM, Context->getLangOpts());
-
- std::string ReserveSize;
- // Handle for-range loop cases.
+ // Add the Reserve size expression to the FixIt
+ // <Name.reserve(>AmountToReserve
if (RangeLoop) {
- // Get the range-expression in a for-range statement represented as
- // `for (range-declarator: range-expression)`.
- StringRef RangeInitExpName =
- Lexer::getSourceText(CharSourceRange::getTokenRange(
- RangeLoop->getRangeInit()->getSourceRange()),
- SM, Context->getLangOpts());
- ReserveSize = (RangeInitExpName + ".size()").str();
+ // Handle for-range loop cases.
+ const Expr *RangeInit = RangeLoop->getRangeInit();
+ if (const auto *SizeMethod =
+ Result.Nodes.getNodeAs<CXXMethodDecl>(RangeLoopSizeMethodName)) {
+ FixIt << GetSourceText(RangeInit) << '.' << SizeMethod->getName() << "()";
+ } else if (const auto *Array = dyn_cast<ArrayType>(
+ RangeInit->getType().IgnoreParens().getTypePtr())) {
+ if (const auto *CArray = dyn_cast<ConstantArrayType>(Array))
+ FixIt << CArray->getSize();
+ else if (const auto *DArray = dyn_cast<DependentSizedArrayType>(Array))
+ FixIt << GetSourceText(DArray->getSizeExpr());
+ else if (const auto *VArray = dyn_cast<VariableArrayType>(Array)) {
+ const Expr *SizeExpr = VArray->getSizeExpr();
+ if (SizeExpr->HasSideEffects(*Context)) {
+ StringRef RangeInitExpName = GetSourceText(RangeInit);
+ FixIt << "sizeof(" << RangeInitExpName << ") / sizeof("
+ << RangeInitExpName << "[0])";
+ } else {
+ FixIt << GetSourceText(VArray->getSizeExpr());
+ }
+ } else
+ // IncompleteArrayType can't get the size
+ return;
+ } else
+ llvm_unreachable("Unknown RangeFor Expression");
} else if (ForLoop) {
// Handle counter-based loop cases.
- StringRef LoopEndSource = Lexer::getSourceText(
- CharSourceRange::getTokenRange(LoopEndExpr->getSourceRange()), SM,
- Context->getLangOpts());
- ReserveSize = std::string(LoopEndSource);
- }
+ const auto *LoopEndExpr = Result.Nodes.getNodeAs<Expr>(LoopEndExprName);
+ assert(LoopEndExpr && "LoopEndExpr null in matched for loop");
+ FixIt << GetSourceText(LoopEndExpr);
+ } else
+ llvm_unreachable("Unknown loop");
- auto Diag = diag(AppendCall->getBeginLoc(),
- "%0 is called inside a loop; consider pre-allocating the "
- "container capacity before the loop")
- << AppendCall->getMethodDecl()->getDeclName();
- if (!ReserveSize.empty()) {
- std::string ReserveStmt =
- (VarName + PartialReserveStmt + "(" + ReserveSize + ");\n").str();
- Diag << FixItHint::CreateInsertion(LoopStmt->getBeginLoc(), ReserveStmt);
- }
+ FixIt << ");\n" << Lexer::getIndentationForLine(LoopStmt->getBeginLoc(), SM);
+
+ auto Diag =
+ diag(AppendCallInfo->Location,
+ "%0 is called inside a loop; consider pre-allocating the "
+ "container capacity before the loop")
+ << AppendCallInfo->Name
+ << FixItHint::CreateInsertion(LoopStmt->getBeginLoc(), FixIt.str());
}
} // namespace performance
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
