From: Joey Watts Date: Fri, 19 Aug 2022 06:57:12 +0000 (+0100) Subject: [clang-tidy] Improve modernize-use-emplace check X-Git-Tag: upstream/17.0.6~36045 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=b8655f7ff286b9ebcd97cdd24b9c8eb5b89b9651;p=platform%2Fupstream%2Fllvm.git [clang-tidy] Improve modernize-use-emplace check This patch improves the modernize-use-emplace check by adding support for detecting inefficient invocations of the `push` and `push_front` methods on STL-style containers and replacing them with their `emplace`-style equivalent. Fixes #56996. Reviewed By: njames93 Differential Revision: https://reviews.llvm.org/D131623 --- diff --git a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp index e6fb4c0..abf5ba9 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp @@ -84,6 +84,10 @@ AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) { 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 @@ UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context) "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 @@ UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context) 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 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) { 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))), @@ -209,6 +222,18 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) { 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( CallEmplacy, HasConstructExprWithValueTypeTypeAsLastArgument, @@ -237,15 +262,29 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) { 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 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { 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 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { 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 @@ void UseEmplaceCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { 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/clang-tidy/modernize/UseEmplaceCheck.h b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h index 779abf6..4fe788e 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h @@ -37,6 +37,8 @@ public: 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/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index fe33383..9e5e176 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -120,6 +120,12 @@ Changes in existing checks ` 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 index f08bfb4..f61b93a 100644 --- 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 @@ modernize-use-emplace ===================== 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 @@ Options 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 index b869816..29ba881 100644 --- 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 @@ public: 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 @@ public: 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 @@ public: 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 @@ class stack { public: using value_type = T; + void push(const T &) {} + void push(T &&) {} + template void emplace(Args &&...args){}; }; @@ -244,6 +256,9 @@ class queue { public: using value_type = T; + void push(const T &) {} + void push(T &&) {} + template void emplace(Args &&...args){}; }; @@ -253,6 +268,9 @@ class priority_queue { public: using value_type = T; + void push(const T &) {} + void push(T &&) {} + template void emplace(Args &&...args){}; }; @@ -667,15 +685,43 @@ void testOtherContainers() { // 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 {