diff --git a/clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.h b/clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.h --- a/clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.h +++ b/clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.h @@ -35,7 +35,11 @@ StringRef VarDeclName, StringRef VarDeclStmtName, const ast_matchers::DeclarationMatcher &AppendMethodDecl, StringRef AppendCallName, ast_matchers::MatchFinder *Finder); - const std::vector 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; diff --git a/clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp b/clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp --- a/clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp @@ -54,6 +54,7 @@ static const char LoopParentName[] = "loop_parent"; static const char VectorVarDeclName[] = "vector_var_decl"; static const char VectorVarDeclStmtName[] = "vector_var_decl_stmt"; +static const char VectorReserveName[] = "vector_reserve_name"; static const char PushBackOrEmplaceBackCallName[] = "append_call"; static const char ProtoVarDeclName[] = "proto_var_decl"; static const char ProtoVarDeclStmtName[] = "proto_var_decl_stmt"; @@ -61,11 +62,32 @@ static const char LoopInitVarName[] = "loop_init_var"; static const char LoopEndExprName[] = "loop_end_expr"; static const char RangeLoopName[] = "for_range_loop"; +static const char RangeLoopSizeMethod[] = "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 supportedContainerTypesMatcher() { - return hasType(cxxRecordDecl(hasAnyName( - "::std::vector", "::std::set", "::std::unordered_set", "::std::map", - "::std::unordered_map", "::std::array", "::std::deque"))); +ast_matchers::internal::Matcher +hasAnyNameStdString(std::vector Names) { + return ast_matchers::internal::Matcher( + new ast_matchers::internal::HasNameMatcher(std::move(Names))); +} + +static std::vector 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)); } } // namespace @@ -73,14 +95,20 @@ 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); } @@ -145,17 +173,30 @@ // FIXME: Support more complex range-expressions. Finder->addMatcher( cxxForRangeStmt( - hasRangeInit(declRefExpr(supportedContainerTypesMatcher())), + hasRangeInit(declRefExpr(hasType(cxxRecordDecl( + hasAnyNameStdString( + parseStringListPair(SupportedDefaultRanges, SupportedRanges)), + hasMethod(cxxMethodDecl(hasAnyNameStdString(parseStringListPair( + "size", SizeNames)), + parameterCountIs(0), isPublic(), + returns(isInteger())) + .bind(RangeLoopSizeMethod)))))), HasInterestingLoopBody, InInterestingCompoundStmt) .bind(RangeLoopName), this); } void InefficientVectorOperationCheck::registerMatchers(MatchFinder *Finder) { - const auto VectorDecl = cxxRecordDecl(hasAnyName(SmallVector( - VectorLikeClasses.begin(), VectorLikeClasses.end()))); - const auto AppendMethodDecl = - cxxMethodDecl(hasAnyName("push_back", "emplace_back")); + const auto VectorDecl = cxxRecordDecl( + hasAnyNameStdString( + parseStringListPair("::std::vector", VectorLikeClasses)), + hasMethod(cxxMethodDecl(hasAnyNameStdString( + parseStringListPair("reserve", ReserveNames)), + isPublic(), parameterCountIs(1), + hasParameter(0, hasType(isInteger()))) + .bind(VectorReserveName))); + const auto AppendMethodDecl = cxxMethodDecl(hasAnyNameStdString( + parseStringListPair("push_back;emplace_back", AppendNames))); AddMatcher(VectorDecl, VectorVarDeclName, VectorVarDeclStmtName, AppendMethodDecl, PushBackOrEmplaceBackCallName, Finder); @@ -224,7 +265,10 @@ std::string PartialReserveStmt; if (VectorAppendCall != nullptr) { - PartialReserveStmt = ".reserve"; + const auto *ReserveMethod = + Result.Nodes.getNodeAs(VectorReserveName); + assert(ReserveMethod && "Reserve Method should not be null"); + PartialReserveStmt = ("." + ReserveMethod->getName()).str(); } else { llvm::StringRef FieldName = ProtoAddFieldCall->getMethodDecl()->getName(); FieldName.consume_front("add_"); @@ -247,7 +291,11 @@ Lexer::getSourceText(CharSourceRange::getTokenRange( RangeLoop->getRangeInit()->getSourceRange()), SM, Context->getLangOpts()); - ReserveSize = (RangeInitExpName + ".size()").str(); + const auto *SizeMethod = + Result.Nodes.getNodeAs(RangeLoopSizeMethod); + assert(SizeMethod && + "SizeMethod should be valid in a matched RangeForLoop"); + ReserveSize = (RangeInitExpName + "." + SizeMethod->getName() + "()").str(); } else if (ForLoop) { // Handle counter-based loop cases. StringRef LoopEndSource = Lexer::getSourceText( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -109,6 +109,10 @@ Changes in existing checks ^^^^^^^^^^^^^^^^^^^^^^^^^^ +- Improved :doc:`performance-inefficient-vector-operation + ` check now has + extra options for custom containers. + - Improved :doc:`readability-qualified-auto ` check now supports a `AddConstToQualified` to enable adding ``const`` qualifiers to variables diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance-inefficient-vector-operation.rst b/clang-tools-extra/docs/clang-tidy/checks/performance-inefficient-vector-operation.rst --- a/clang-tools-extra/docs/clang-tidy/checks/performance-inefficient-vector-operation.rst +++ b/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 diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-inefficient-vector-operation.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-inefficient-vector-operation.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/performance-inefficient-vector-operation.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance-inefficient-vector-operation.cpp @@ -1,7 +1,12 @@ // 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 +364,160 @@ } } } + +namespace OptionsValidMatchDefault { +template +class MyContainer { +public: + unsigned size(); + T *begin() const; + T *end() const; + void push_back(const T &); + void reserve(unsigned); +}; + +void foo(const MyContainer &C) { + MyContainer 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 +class MyContainer { +public: + unsigned Size(); + T *begin() const; + T *end() const; + void PushBack(const T &); + void Reserve(unsigned); +}; + +void foo(const MyContainer &C) { + MyContainer 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 +class MyUContainer { +public: + unsigned size(); + T *begin() const; + T *end() const; + void push_back(const T &); + void reserve(unsigned); +}; + +void foo(const MyUContainer &C) { + // MyUContainer isn't specified as a VectorLikeClass in the Config Options + MyUContainer 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 +class MyContainer { + unsigned size(); + +public: + T *begin() const; + T *end() const; + void push_back(const T &); + void reserve(unsigned); +}; + +void foo(const MyContainer &C) { + // MyContainer::size is private, so calling it will be invalid + MyContainer 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 +class MyContainer { +public: + unsigned size(); + T *begin() const; + T *end() const; + void push_back(const T &); + +private: + void reserve(unsigned); +}; + +void foo(const MyContainer &C) { + // MyContainer::reserve is private, so calling it will be invalid + MyContainer 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 +class MyContainer { +public: + char *size(); + T *begin() const; + T *end() const; + void push_back(const T &); + void reserve(unsigned); +}; + +void foo(const MyContainer &C) { + // MyContainer::size doesn't return an integral type(char *), so ignore this class + MyContainer 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 +class MyContainer { +public: + unsigned size(); + T *begin() const; + T *end() const; + void push_back(const T &); + void reserve(); +}; + +void foo(const MyContainer &C) { + // MyContainer::reserve doesn't take a single integral argument, so ignore this class + MyContainer 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