diff --git a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h --- a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h @@ -37,6 +37,8 @@ private: const bool IgnoreImplicitConstructors; const std::vector ContainersWithPushBack; + const std::vector ContainersWithPush; + const std::vector ContainersWithPushFront; const std::vector SmartPointers; const std::vector TupleTypes; const std::vector TupleMakeFunctions; diff --git a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp --- a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp @@ -84,6 +84,10 @@ const auto DefaultContainersWithPushBack = "::std::vector; ::std::list; ::std::deque"; +const auto DefaultContainersWithPush = + "::std::stack; ::std::queue; ::std::priority_queue"; +const auto DefaultContainersWithPushFront = + "::std::forward_list; ::std::list; ::std::deque"; const auto DefaultSmartPointers = "::std::shared_ptr; ::std::unique_ptr; ::std::auto_ptr; ::std::weak_ptr"; const auto DefaultTupleTypes = "::std::pair; ::std::tuple"; @@ -109,6 +113,10 @@ "IgnoreImplicitConstructors", false)), ContainersWithPushBack(utils::options::parseStringList(Options.get( "ContainersWithPushBack", DefaultContainersWithPushBack))), + ContainersWithPush(utils::options::parseStringList( + Options.get("ContainersWithPush", DefaultContainersWithPush))), + ContainersWithPushFront(utils::options::parseStringList(Options.get( + "ContainersWithPushFront", DefaultContainersWithPushFront))), SmartPointers(utils::options::parseStringList( Options.get("SmartPointers", DefaultSmartPointers))), TupleTypes(utils::options::parseStringList( @@ -120,9 +128,6 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) { // FIXME: Bunch of functionality that could be easily added: - // + add handling of `push_front` for std::forward_list, std::list - // and std::deque. - // + add handling of `push` for std::stack, std::queue, std::priority_queue // + add handling of `insert` for stl associative container, but be careful // because this requires special treatment (it could cause performance // regression) @@ -131,6 +136,14 @@ hasDeclaration(functionDecl(hasName("push_back"))), on(hasType(cxxRecordDecl(hasAnyName(ContainersWithPushBack))))); + auto CallPush = cxxMemberCallExpr( + hasDeclaration(functionDecl(hasName("push"))), + on(hasType(cxxRecordDecl(hasAnyName(ContainersWithPush))))); + + auto CallPushFront = cxxMemberCallExpr( + hasDeclaration(functionDecl(hasName("push_front"))), + on(hasType(cxxRecordDecl(hasAnyName(ContainersWithPushFront))))); + auto CallEmplacy = cxxMemberCallExpr( hasDeclaration( functionDecl(hasAnyNameIgnoringTemplates(EmplacyFunctions))), @@ -208,6 +221,18 @@ .bind("push_back_call")), this); + Finder->addMatcher( + traverse(TK_AsIs, cxxMemberCallExpr(CallPush, has(SoughtParam), + unless(isInTemplateInstantiation())) + .bind("push_call")), + this); + + Finder->addMatcher( + traverse(TK_AsIs, cxxMemberCallExpr(CallPushFront, has(SoughtParam), + unless(isInTemplateInstantiation())) + .bind("push_front_call")), + this); + Finder->addMatcher( traverse(TK_AsIs, cxxMemberCallExpr( @@ -237,15 +262,29 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { const auto *PushBackCall = Result.Nodes.getNodeAs("push_back_call"); + const auto *PushCall = Result.Nodes.getNodeAs("push_call"); + const auto *PushFrontCall = + Result.Nodes.getNodeAs("push_front_call"); const auto *EmplacyCall = Result.Nodes.getNodeAs("emplacy_call"); const auto *CtorCall = Result.Nodes.getNodeAs("ctor"); const auto *MakeCall = Result.Nodes.getNodeAs("make"); - assert((PushBackCall || EmplacyCall) && "No call matched"); - assert((CtorCall || MakeCall) && "No push_back parameter matched"); + const CXXMemberCallExpr *Call = [&]() { + if (PushBackCall) { + return PushBackCall; + } + if (PushCall) { + return PushCall; + } + if (PushFrontCall) { + return PushFrontCall; + } + return EmplacyCall; + }(); - const CXXMemberCallExpr *Call = PushBackCall ? PushBackCall : EmplacyCall; + assert(Call && "No call matched"); + assert((CtorCall || MakeCall) && "No push_back parameter matched"); if (IgnoreImplicitConstructors && CtorCall && CtorCall->getNumArgs() >= 1 && CtorCall->getArg(0)->getSourceRange() == CtorCall->getSourceRange()) @@ -255,11 +294,19 @@ Call->getExprLoc(), Call->getArg(0)->getExprLoc()); auto Diag = - PushBackCall - ? diag(Call->getExprLoc(), "use emplace_back instead of push_back") - : diag(CtorCall ? CtorCall->getBeginLoc() : MakeCall->getBeginLoc(), - "unnecessary temporary object created while calling " + - Call->getMethodDecl()->getName().str()); + EmplacyCall + ? diag(CtorCall ? CtorCall->getBeginLoc() : MakeCall->getBeginLoc(), + "unnecessary temporary object created while calling %0") + : diag(Call->getExprLoc(), "use emplace%select{|_back|_front}0 " + "instead of push%select{|_back|_front}0"); + if (EmplacyCall) + Diag << Call->getMethodDecl()->getName(); + else if (PushCall) + Diag << 0; + else if (PushBackCall) + Diag << 1; + else + Diag << 2; if (FunctionNameSourceRange.getBegin().isMacroID()) return; @@ -268,6 +315,14 @@ const char *EmplacePrefix = MakeCall ? "emplace_back" : "emplace_back("; Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, EmplacePrefix); + } else if (PushCall) { + const char *EmplacePrefix = MakeCall ? "emplace" : "emplace("; + Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, + EmplacePrefix); + } else if (PushFrontCall) { + const char *EmplacePrefix = MakeCall ? "emplace_front" : "emplace_front("; + Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, + EmplacePrefix); } const SourceRange CallParensRange = @@ -302,6 +357,10 @@ Options.store(Opts, "IgnoreImplicitConstructors", IgnoreImplicitConstructors); Options.store(Opts, "ContainersWithPushBack", utils::options::serializeStringList(ContainersWithPushBack)); + Options.store(Opts, "ContainersWithPush", + utils::options::serializeStringList(ContainersWithPush)); + Options.store(Opts, "ContainersWithPushFront", + utils::options::serializeStringList(ContainersWithPushFront)); Options.store(Opts, "SmartPointers", utils::options::serializeStringList(SmartPointers)); Options.store(Opts, "TupleTypes", 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 @@ -120,6 +120,12 @@ ` check. Partial support for C++14 signal handler rules was added. Bug report generation was improved. + +- Improved `modernize-use-emplace `_ check. + + The check now supports detecting inefficient invocations of ``push`` and + ``push_front`` on STL-style containers and replacing them with ``emplace`` + or ``emplace_front``. Removed checks ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst --- a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst @@ -4,16 +4,22 @@ ===================== The check flags insertions to an STL-style container done by calling the -``push_back`` method with an explicitly-constructed temporary of the container -element type. In this case, the corresponding ``emplace_back`` method -results in less verbose and potentially more efficient code. -Right now the check doesn't support ``push_front`` and ``insert``. -It also doesn't support ``insert`` functions for associative containers -because replacing ``insert`` with ``emplace`` may result in +``push_back``, ``push``, or ``push_front`` methods with an +explicitly-constructed temporary of the container element type. In this case, +the corresponding ``emplace`` equivalent methods result in less verbose and +potentially more efficient code. Right now the check doesn't support +``insert``. It also doesn't support ``insert`` functions for associative +containers because replacing ``insert`` with ``emplace`` may result in `speed regression `_, but it might get support with some addition flag in the future. -By default only ``std::vector``, ``std::deque``, ``std::list`` are considered. -This list can be modified using the :option:`ContainersWithPushBack` option. +The :option:`ContainersWithPushBack`, :option:`ContainersWithPush`, and +:option:`ContainersWithPushFront` options are used to specify the container +types that support the ``push_back``, ``push``, and ``push_front`` operations +respectively. The default values for these options are as follows: + +* :option:`ContainersWithPushBack`: ``std::vector``, ``std::deque``, and ``std::list``. +* :option:`ContainersWithPush`: ``std::stack``, ``std::queue``, and ``std::priority_queue``. +* :option:`ContainersWithPushFront`: ``std::forward_list``, ``std::list``, and ``std::deque``. This check also reports when an ``emplace``-like method is improperly used, for example using ``emplace_back`` while also calling a constructor. This @@ -116,6 +122,16 @@ Semicolon-separated list of class names of custom containers that support ``push_back``. +.. option:: ContainersWithPush + + Semicolon-separated list of class names of custom containers that support + ``push``. + +.. option:: ContainersWithPushFront + + Semicolon-separated list of class names of custom containers that support + ``push_front``. + .. option:: IgnoreImplicitConstructors When `true`, the check will ignore implicitly constructed arguments of diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp @@ -62,6 +62,9 @@ class const_iterator {}; const_iterator begin() { return const_iterator{}; } + void push_front(const T &) {} + void push_front(T &&) {} + void push_back(const T &) {} void push_back(T &&) {} @@ -86,6 +89,9 @@ void push_back(const T &) {} void push_back(T &&) {} + void push_front(const T &) {} + void push_front(T &&) {} + template iterator emplace(const_iterator pos, Args &&...args){}; template @@ -104,6 +110,9 @@ class const_iterator {}; const_iterator begin() { return const_iterator{}; } + void push_front(const T &) {} + void push_front(T &&) {} + template void emplace_front(Args &&...args){}; template @@ -235,6 +244,9 @@ public: using value_type = T; + void push(const T &) {} + void push(T &&) {} + template void emplace(Args &&...args){}; }; @@ -244,6 +256,9 @@ public: using value_type = T; + void push(const T &) {} + void push(T &&) {} + template void emplace(Args &&...args){}; }; @@ -253,6 +268,9 @@ public: using value_type = T; + void push(const T &) {} + void push(T &&) {} + template void emplace(Args &&...args){}; }; @@ -667,15 +685,43 @@ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back // CHECK-FIXES: l.emplace_back(42, 41); + l.push_front(Something(42, 41)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_front + // CHECK-FIXES: l.emplace_front(42, 41); + std::deque d; d.push_back(Something(42)); // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back // CHECK-FIXES: d.emplace_back(42); + d.push_front(Something(42)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_front + // CHECK-FIXES: d.emplace_front(42); + llvm::LikeASmallVector ls; ls.push_back(Something(42)); // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back // CHECK-FIXES: ls.emplace_back(42); + + std::stack s; + s.push(Something(42, 41)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace + // CHECK-FIXES: s.emplace(42, 41); + + std::queue q; + q.push(Something(42, 41)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace + // CHECK-FIXES: q.emplace(42, 41); + + std::priority_queue pq; + pq.push(Something(42, 41)); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace + // CHECK-FIXES: pq.emplace(42, 41); + + std::forward_list fl; + fl.push_front(Something(42, 41)); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_front + // CHECK-FIXES: fl.emplace_front(42, 41); } class IntWrapper {