[clang-tidy] Improve modernize-use-emplace check

[joeywatts]

Summary

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.

[joeywatts]

Update documentation

[Eugene.Zelenko]

Please mention changes in Release Notes.

[joeywatts]

Updated commit title/description.

[joeywatts]

Add changes to release notes.

[joeywatts]

Update commit title/message

[njames93]

Just a general drive by comment here and doesn't affect this patch.
This specifying containers logic is a little verbose, There may be a case to deprecate most of these options and just detect containers with an equivalent emplace* method at runtime.

clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp

297–310

327

332

[joeywatts]

In D131623#3716227, @njames93 wrote:

Just a general drive by comment here and doesn't affect this patch.
This specifying containers logic is a little verbose, There may be a case to deprecate most of these options and just detect containers with an equivalent emplace* method at runtime.

I thought the same thing, but wasn't sure what the stance is on breaking changes here: would it be a problem if the existing "ContainersWithPush" option were removed in favor of a more generic option for a list of containers?

EDIT: ah, I see you mean not specifying any list of containers at all and just detecting if the type has an equivalent emplace-style method.

[joeywatts]

Address code review comments

[joeywatts]

Thanks for the review @njames93! This is my first contribution so I don't think I have permission to land this myself, is there someone that can do that for me?

[njames93]

In D131623#3730833, @joeywatts wrote:

Thanks for the review @njames93! This is my first contribution so I don't think I have permission to land this myself, is there someone that can do that for me?

Sure, which email would you like me to use for the commit?

[joeywatts]

In D131623#3731106, @njames93 wrote:

In D131623#3730833, @joeywatts wrote:

Thanks for the review @njames93! This is my first contribution so I don't think I have permission to land this myself, is there someone that can do that for me?

Sure, which email would you like me to use for the commit?

Please use [e-mail]!

clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h

public:
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
 
 private:
   const bool IgnoreImplicitConstructors;
   const std::vector<StringRef> ContainersWithPushBack;
+  const std::vector<StringRef> ContainersWithPush;
+  const std::vector<StringRef> ContainersWithPushFront;
   const std::vector<StringRef> SmartPointers;
   const std::vector<StringRef> TupleTypes;
   const std::vector<StringRef> TupleMakeFunctions;
   const std::vector<StringRef> EmplacyFunctions;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_EMPLACE_H

clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp

 }
 
 AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
   return Node.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";
 const auto DefaultTupleMakeFunctions = "::std::make_pair; ::std::make_tuple";
 const auto DefaultEmplacyFunctions =
     "vector::emplace_back; vector::emplace;"
     "deque::emplace; deque::emplace_front; deque::emplace_back;"
     "forward_list::emplace_after; forward_list::emplace_front;"
const auto DefaultEmplacyFunctions =
     "stack::emplace; queue::emplace; priority_queue::emplace";
 } // namespace
 
 UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context), IgnoreImplicitConstructors(Options.get(
                                          "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(
           Options.get("TupleTypes", DefaultTupleTypes))),
       TupleMakeFunctions(utils::options::parseStringList(
           Options.get("TupleMakeFunctions", DefaultTupleMakeFunctions))),
       EmplacyFunctions(utils::options::parseStringList(
           Options.get("EmplacyFunctions", DefaultEmplacyFunctions))) {}
 
 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)
   // + match for emplace calls that should be replaced with insertion
   auto CallPushBack = cxxMemberCallExpr(
       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))),
       on(hasType(cxxRecordDecl(has(typedefNameDecl(
           hasName("value_type"), hasType(type(hasUnqualifiedDesugaredType(
                                      recordType().bind("value_type"))))))))));
 
   // We can't replace push_backs of smart pointer because
void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
 
   Finder->addMatcher(
       traverse(TK_AsIs, cxxMemberCallExpr(CallPushBack, has(SoughtParam),
                                           unless(isInTemplateInstantiation()))
                             .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(
                    CallEmplacy, HasConstructExprWithValueTypeTypeAsLastArgument,
                    hasSameNumArgsAsDeclNumParams(),
                    unless(isInTemplateInstantiation()))
                    .bind("emplacy_call")),
       this);
 
Finder->addMatcher(
               unless(isInTemplateInstantiation()))
               .bind("emplacy_call")),
       this);
 }
 
 void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *PushBackCall =
       Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_back_call");
+  const auto *PushCall = Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_call");
+  const auto *PushFrontCall =
+      Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_front_call");
   const auto *EmplacyCall =
       Result.Nodes.getNodeAs<CXXMemberCallExpr>("emplacy_call");
   const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctor");
   const auto *MakeCall = Result.Nodes.getNodeAs<CallExpr>("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())
     return;
 
   const auto FunctionNameSourceRange = CharSourceRange::getCharRange(
       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;
 
   if (PushBackCall) {
     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 =
       MakeCall ? SourceRange(MakeCall->getCallee()->getEndLoc(),
                              MakeCall->getRParenLoc())
                : CtorCall->getParenOrBraceRange();
 
   // Finish if there is no explicit constructor call.
   if (CallParensRange.getBegin().isInvalid())
     return;
 
   const SourceLocation ExprBegin =
       CtorCall ? CtorCall->getExprLoc() : MakeCall->getExprLoc();
 
   // Range for constructor name and opening brace.
Diag << FixItHint::CreateRemoval(
                                       MakeCall->getArg(0)->getBeginLoc()));
   }
 }
 
 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",
                 utils::options::serializeStringList(TupleTypes));
   Options.store(Opts, "TupleMakeFunctions",
                 utils::options::serializeStringList(TupleMakeFunctions));
   Options.store(Opts, "EmplacyFunctions",
                 utils::options::serializeStringList(EmplacyFunctions));
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Improved :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone/signal-handler>` check. Partial
   support for C++14 signal handler rules was added. Bug report generation was
   improved.
   
+- Improved `modernize-use-emplace <clang-tidy/checks/modernize/use-emplace.html>`_ 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
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst

 .. title:: clang-tidy - modernize-use-emplace
 
 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 <https://htmlpreview.github.io/?https://github.com/HowardHinnant/papers/blob/master/insert_vs_emplace.html>`_, 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
 creates a temporary that requires at best a move and at worst a copy. Almost all
 ``emplace``-like functions in the STL are covered by this, with ``try_emplace``
 on ``std::map`` and ``std::unordered_map`` being the exception as it behaves
 slightly differently than all the others. More containers can be added with the
 :option:`EmplacyFunctions` option, so long as the container defines a

 Options
 -------
 
 .. option:: ContainersWithPushBack
 
    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
     ``push_back``, e.g.
 
     .. code-block:: c++
 
         std::vector<std::string> v;

clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp

 class list {
 public:
   using value_type = T;
 
   class iterator {};
   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 &&) {}
 
   template <typename... Args>
   iterator emplace(const_iterator pos, Args &&...args){};
   template <typename... Args>
   void emplace_back(Args &&... args){};
   template <typename... Args>
   void emplace_front(Args &&...args){};
   ~list();
 };
 
 template <typename T>
 class deque {
 public:
   using value_type = T;
 
   class iterator {};
   class const_iterator {};
   const_iterator begin() { return const_iterator{}; }
 
   void push_back(const T &) {}
   void push_back(T &&) {}
 
+  void push_front(const T &) {}
+  void push_front(T &&) {}
+
   template <typename... Args>
   iterator emplace(const_iterator pos, Args &&...args){};
   template <typename... Args>
   void emplace_back(Args &&... args){};
   template <typename... Args>
   void emplace_front(Args &&...args){};
   ~deque();
 };
 
 template <typename T>
 class forward_list {
 public:
   using value_type = T;
 
   class iterator {};
   class const_iterator {};
   const_iterator begin() { return const_iterator{}; }
 
+  void push_front(const T &) {}
+  void push_front(T &&) {}
+
   template <typename... Args>
   void emplace_front(Args &&...args){};
   template <typename... Args>
   iterator emplace_after(const_iterator pos, Args &&...args){};
 };
 
 template <typename T>
 class set {
public:
   iterator emplace_hint(const_iterator pos, Args &&...args){};
 };
 
 template <typename T>
 class stack {
 public:
   using value_type = T;
 
+  void push(const T &) {}
+  void push(T &&) {}
+
   template <typename... Args>
   void emplace(Args &&...args){};
 };
 
 template <typename T>
 class queue {
 public:
   using value_type = T;
 
+  void push(const T &) {}
+  void push(T &&) {}
+
   template <typename... Args>
   void emplace(Args &&...args){};
 };
 
 template <typename T>
 class priority_queue {
 public:
   using value_type = T;
 
+  void push(const T &) {}
+  void push(T &&) {}
+
   template <typename... Args>
   void emplace(Args &&...args){};
 };
 
 template <typename T>
 struct remove_reference { using type = T; };
 template <typename T>
 struct remove_reference<T &> { using type = T; };

 }
 
 void testOtherContainers() {
   std::list<Something> l;
   l.push_back(Something(42, 41));
   // 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<Something> 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<Something> ls;
   ls.push_back(Something(42));
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
   // CHECK-FIXES: ls.emplace_back(42);
+
+  std::stack<Something> s;
+  s.push(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace
+  // CHECK-FIXES: s.emplace(42, 41);
+
+  std::queue<Something> q;
+  q.push(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace
+  // CHECK-FIXES: q.emplace(42, 41);
+
+  std::priority_queue<Something> pq;
+  pq.push(Something(42, 41));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace
+  // CHECK-FIXES: pq.emplace(42, 41);
+
+  std::forward_list<Something> 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 {
 public:
   IntWrapper(int x) : value(x) {}
   IntWrapper operator+(const IntWrapper other) const {
     return IntWrapper(value + other.value);
   }