[clang-tidy] modernize-use-emplace: Remove unnecessary make_tuple calls

[kuhar]

Summary

This patch makes modernize-use-emplace remove unnecessary make_ calls from push_back calls and turn them into emplace_back -- the same way make_pair calls are handled.
Custom make_ calls can be removed for custom tuple-like types -- two new options that control that are TupleTypes and TupleMakeFunctions. By default, the check removes calls to std::make_pair and std::make_tuple.

Eq.

std::vector<std::tuple<int, char, bool>> v;
v.push_back(std::make_tuple(1, 'A', true)); // --> v.emplace_back(1, 'A', true);

[kuhar]

Cosmetic changes.

[JonasToth]

Nothing to complain from my side.
Would it make sense to add boost pairs/tuples support?
I could imagine modernize checks that would transform boost structures into the stl ones, but that's another discussion probably.

Something for a later check: the same logic should apply for insert and emplace, we should add support for these methods too.

[kuhar]

In D32690#741952, @JonasToth wrote:

Would it make sense to add boost pairs/tuples support?

I think it can be added the same way it is possible to specify smart pointers and vector-like containers now. It would require users to provide both types and make functions (eg. thingy and make_thingy). I'm not sure if that would be any useful in practise, though.

Something for a later check: the same logic should apply for insert and emplace, we should add support for these methods too.

Yeah, I plan to do that a few patches from now, after other things are improved.

[kuhar]

I went ahead and generalized the patch to support custom tuple-like types and custom make_ functions.
I added a bunch of tests and updated the docs.

Let me know what you think.

[JonasToth]

i like the generalization.

clang-tidy/modernize/UseEmplaceCheck.cpp

115 ↗

(On Diff #97299)

i think this); should move up a line. clang-format seems to make this all the time.

[kuhar]

I updated the patch against the current trunk.

I also run the modernize-use-emplace check on llvm and verified that there are no new regressions after applying fixits.

[JDevlieghere]

No comments from me either, looks good!

[Prazek]

LGTM

[kuhar]

The patch broke the spinx docs build and I had to revert it in r303140.
I fixed the docs file problems.

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

public:
   UseEmplaceCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
 
 private:
   std::vector<std::string> ContainersWithPushBack;
   std::vector<std::string> SmartPointers;
+  std::vector<std::string> TupleTypes;
+  std::vector<std::string> TupleMakeFunctions;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_EMPLACE_H

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

 AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
   return Node.hasExplicitTemplateArgs();
 }
 
 const auto DefaultContainersWithPushBack =
     "::std::vector; ::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";
 } // namespace
 
 UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       ContainersWithPushBack(utils::options::parseStringList(Options.get(
           "ContainersWithPushBack", DefaultContainersWithPushBack))),
       SmartPointers(utils::options::parseStringList(
-          Options.get("SmartPointers", DefaultSmartPointers))) {}
+          Options.get("SmartPointers", DefaultSmartPointers))),
+      TupleTypes(utils::options::parseStringList(
+          Options.get("TupleTypes", DefaultTupleTypes))),
+      TupleMakeFunctions(utils::options::parseStringList(
+          Options.get("TupleMakeFunctions", DefaultTupleMakeFunctions))) {}
 
 void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus11)
     return;
 
   // FIXME: Bunch of functionality that could be easily added:
   // + add handling of `push_front` for std::forward_list, std::list
   // and std::deque.
void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
   auto SoughtConstructExpr =
       cxxConstructExpr(
           unless(anyOf(IsCtorOfSmartPtr, HasInitList, BitFieldAsArgument,
                        InitializerListAsArgument, NewExprAsArgument,
                        ConstructingDerived, IsPrivateCtor)))
           .bind("ctor");
   auto HasConstructExpr = has(ignoringImplicit(SoughtConstructExpr));
 
-  auto MakePair = ignoringImplicit(
-      callExpr(callee(expr(ignoringImplicit(
-          declRefExpr(unless(hasExplicitTemplateArgs()),
-                      to(functionDecl(hasName("::std::make_pair"))))
-      )))).bind("make_pair"));
+  auto MakeTuple = ignoringImplicit(
+      callExpr(
+          callee(expr(ignoringImplicit(declRefExpr(
+              unless(hasExplicitTemplateArgs()),
+              to(functionDecl(hasAnyName(SmallVector<StringRef, 2>(
+                  TupleMakeFunctions.begin(), TupleMakeFunctions.end())))))))))
+          .bind("make"));
 
-  // make_pair can return type convertible to container's element type.
+  // make_something can return type convertible to container's element type.
   // Allow the conversion only on containers of pairs.
-  auto MakePairCtor = ignoringImplicit(cxxConstructExpr(
-      has(materializeTemporaryExpr(MakePair)),
-      hasDeclaration(cxxConstructorDecl(ofClass(hasName("::std::pair"))))));
+  auto MakeTupleCtor = ignoringImplicit(cxxConstructExpr(
+      has(materializeTemporaryExpr(MakeTuple)),
+      hasDeclaration(cxxConstructorDecl(ofClass(hasAnyName(
+          SmallVector<StringRef, 2>(TupleTypes.begin(), TupleTypes.end())))))));
 
   auto SoughtParam = materializeTemporaryExpr(
-      anyOf(has(MakePair), has(MakePairCtor),
+      anyOf(has(MakeTuple), has(MakeTupleCtor),
             HasConstructExpr, has(cxxFunctionalCastExpr(HasConstructExpr))));
 
   Finder->addMatcher(cxxMemberCallExpr(CallPushBack, has(SoughtParam),
                                        unless(isInTemplateInstantiation()))
                          .bind("call"),
                      this);
 }
 
 void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Call = Result.Nodes.getNodeAs<CXXMemberCallExpr>("call");
   const auto *InnerCtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctor");
-  const auto *MakePairCall = Result.Nodes.getNodeAs<CallExpr>("make_pair");
-  assert((InnerCtorCall || MakePairCall) && "No push_back parameter matched");
+  const auto *MakeCall = Result.Nodes.getNodeAs<CallExpr>("make");
+  assert((InnerCtorCall || MakeCall) && "No push_back parameter matched");
 
   const auto FunctionNameSourceRange = CharSourceRange::getCharRange(
       Call->getExprLoc(), Call->getArg(0)->getExprLoc());
 
   auto Diag = diag(Call->getExprLoc(), "use emplace_back instead of push_back");
 
   if (FunctionNameSourceRange.getBegin().isMacroID())
     return;
 
-  const auto *EmplacePrefix = MakePairCall ? "emplace_back" : "emplace_back(";
+  const auto *EmplacePrefix = MakeCall ? "emplace_back" : "emplace_back(";
   Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, EmplacePrefix);
 
   const SourceRange CallParensRange =
-      MakePairCall ? SourceRange(MakePairCall->getCallee()->getLocEnd(),
-                                 MakePairCall->getRParenLoc())
-                   : InnerCtorCall->getParenOrBraceRange();
+      MakeCall ? SourceRange(MakeCall->getCallee()->getLocEnd(),
+                             MakeCall->getRParenLoc())
 
   // Finish if there is no explicit constructor call.
   if (CallParensRange.getBegin().isInvalid())
     return;
 
   const SourceLocation ExprBegin =
-      MakePairCall ? MakePairCall->getExprLoc() : InnerCtorCall->getExprLoc();
+      MakeCall ? MakeCall->getExprLoc() : InnerCtorCall->getExprLoc();
 
   // Range for constructor name and opening brace.
   const auto ParamCallSourceRange =
       CharSourceRange::getTokenRange(ExprBegin, CallParensRange.getBegin());
 
   Diag << FixItHint::CreateRemoval(ParamCallSourceRange)
        << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
            CallParensRange.getEnd(), CallParensRange.getEnd()));
 }
 
 void UseEmplaceCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ContainersWithPushBack",
                 utils::options::serializeStringList(ContainersWithPushBack));
   Options.store(Opts, "SmartPointers",
                 utils::options::serializeStringList(SmartPointers));
+  Options.store(Opts, "TupleTypes",
+                utils::options::serializeStringList(TupleTypes));
+  Options.store(Opts, "TupleMakeFunctions",
+                utils::options::serializeStringList(TupleMakeFunctions));
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- New `modernize-return-braced-init-list
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-return-braced-init-list.html>`_ check
 
   Finds and replaces explicit calls to the constructor in a return statement by
   a braced initializer list so that the return type is not needlessly repeated.
   
 - Improved `modernize-use-emplace
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html>`_ check
 
-  Removes unnecessary std::make_pair calls in push_back(std::make_pair(a, b)) calls and turns them
-  into emplace_back(a, b).
+  Removes unnecessary ``std::make_pair`` and ``std::make_tuple`` calls in
+  push_back calls and turns them into emplace_back. The check now also is able
+  to remove user-defined make functions from ``push_back`` calls on containers
+  of custom tuple-like types by providing `TupleTypes` and `TupleMakeFunctions`.
 
 - New `performance-inefficient-vector-operation
   <http://clang.llvm.org/extra/clang-tidy/checks/performance-inefficient-vector-operation.html>`_ check
 
   Finds possible inefficient vector operations in for loops that may cause
   unnecessary memory reallocations.
   
 - Added `ParameterThreshold` to `readability-function-size

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

.. code-block:: c++
 
     std::vector<MyClass> v;
     v.emplace_back(21, 37);
 
     std::vector<std::pair<int, int>> w;
     w.emplace_back(21, 37);
     w.emplace_back(21L, 37L);
 
+By default, the check is able to remove unnecessary ``std::make_pair`` and
+``std::make_tuple`` calls from ``push_back`` calls on containers of
+``std::pair``s and ``std::tuple``s. Custom tuple-like types can be modified by
+the :option:`TupleTypes` option; custom make functions can be modified by the
+:option:`TupleMakeFunctions` option.
+
 The other situation is when we pass arguments that will be converted to a type
 inside a container.
 
 Before:
 
 .. code-block:: c++
 
     std::vector<boost::optional<std::string> > v;

 .. option:: ContainersWithPushBack
 
    Semicolon-separated list of class names of custom containers that support
    ``push_back``.
 
 .. option:: SmartPointers
 
    Semicolon-separated list of class names of custom smart pointers.
+
+.. opion:: TupleTypes
+
+    Semicolon-separated list of ``std::tuple``-like class names.
+
+.. option:: TupleMakeFunctions
+
+    Semicolon-separated list of ``std::make_tuple``-like function names. Those
+    function calls will be removed from ``push_back`` calls and turned into
+    ``emplace_back``.
+
+Example
+^^^^^^^
+
+.. code-block:: c++
+
+  std::vector<MyTuple<int, bool, char>> x;
+  x.push_back(MakeMyTuple(1, false, 'x'));
+
+transforms to:
+
+.. code-block:: c++
+
+  std::vector<MyTuple<int, bool, char>> x;
+  x.emplace_back(1, false, 'x');
+
+when :option:`TupleTypes` is set to ``MyTuple`` and :option:`TupleMakeFunctions`
+is set to ``MakeMyTuple``.

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

 // RUN: %check_clang_tidy %s modernize-use-emplace %t -- \
 // RUN:   -config="{CheckOptions: \
 // RUN:             [{key: modernize-use-emplace.ContainersWithPushBack, \
-// RUN:               value: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector'}]}" -- -std=c++11
+// RUN:               value: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector'}, \
+// RUN:              {key: modernize-use-emplace.TupleTypes, \
+// RUN:               value: '::std::pair; std::tuple; ::test::Single'}, \
+// RUN:              {key: modernize-use-emplace.TupleMakeFunctions, \
+// RUN:               value: '::std::make_pair; ::std::make_tuple; ::test::MakeSingle'}] \
+// RUN:             }" -- -std=c++11
 
 namespace std {
 template <typename>
 class initializer_list
 {
 public:
   initializer_list() noexcept {}
 };
public:
   void push_back(const T &) {}
   void push_back(T &&) {}
 
   template <typename... Args>
   void emplace_back(Args &&... args){};
   ~deque();
 };
 
-template <typename T1, typename T2>
-class pair {
+template <typename T> struct remove_reference { using type = T; };
+template <typename T> struct remove_reference<T &> { using type = T; };
+template <typename T> struct remove_reference<T &&> { using type = T; };
+
+template <typename T1, typename T2> class pair {
 public:
   pair() = default;
   pair(const pair &) = default;
   pair(pair &&) = default;
 
   pair(const T1 &, const T2 &) {}
   pair(T1 &&, T2 &&) {}
 
-  template <class U1, class U2>
-  pair(const pair<U1, U2> &p){};
-  template <class U1, class U2>
-  pair(pair<U1, U2> &&p){};
+  template <typename U1, typename U2> pair(const pair<U1, U2> &){};
+  template <typename U1, typename U2> pair(pair<U1, U2> &&){};
 };
 
 template <typename T1, typename T2>
-pair<T1, T2> make_pair(T1&&, T2&&) {
+pair<typename remove_reference<T1>::type, typename remove_reference<T2>::type>
+make_pair(T1 &&, T2 &&) {
   return {};
 };
 
+template <typename... Ts> class tuple {
+public:
+  tuple() = default;
+  tuple(const tuple &) = default;
+  tuple(tuple &&) = default;
+
+  tuple(const Ts &...) {}
+  tuple(Ts &&...) {}
+
+  template <typename... Us> tuple(const tuple<Us...> &){};
+  template <typename... Us> tuple(tuple<Us...> &&) {}
+
+  template <typename U1, typename U2> tuple(const pair<U1, U2> &) {
+    static_assert(sizeof...(Ts) == 2, "Wrong tuple size");
+  };
+  template <typename U1, typename U2> tuple(pair<U1, U2> &&) {
+    static_assert(sizeof...(Ts) == 2, "Wrong tuple size");
+  };
+};
+
+template <typename... Ts>
+tuple<typename remove_reference<Ts>::type...> make_tuple(Ts &&...) {
+  return {};
+}
+
 template <typename T>
 class unique_ptr {
 public:
   explicit unique_ptr(T *) {}
   ~unique_ptr();
 };
 } // namespace std
 
void testPair() {
   // CHECK-FIXES: v.emplace_back(g.getPair());
 
   std::vector<std::pair<Something, Zoz>> v2;
   v2.push_back(std::pair<Something, Zoz>(Something(42, 42), Zoz(Something(21, 37))));
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
   // CHECK-FIXES: v2.emplace_back(Something(42, 42), Zoz(Something(21, 37)));
 }
 
+void testTuple() {
+  std::vector<std::tuple<bool, char, int>> v;
+  v.push_back(std::tuple<bool, char, int>(false, 'x', 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(false, 'x', 1);
+
+  v.push_back(std::tuple<bool, char, int>{false, 'y', 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(false, 'y', 2);
+
+  v.push_back({true, 'z', 3});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(true, 'z', 3);
+
+  std::vector<std::tuple<int, bool>> x;
+  x.push_back(std::make_pair(1, false));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(1, false);
+
+  x.push_back(std::make_pair(2LL, 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(2LL, 1);
+}
+
 struct Base {
   Base(int, int *, int = 42);
 };
 
 struct Derived : Base {
   Derived(int *, Something) : Base(42, nullptr) {}
 };
 
void testMakePair() {
   };
   std::vector<Y> y;
   y.push_back(std::make_pair(2, 3));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: y.emplace_back(std::make_pair(2, 3));
   // make_pair cannot be removed here, as Y is not constructible with two ints.
 }
 
+void testMakeTuple() {
+  std::vector<std::tuple<int, bool, char>> v;
+  v.push_back(std::make_tuple(1, true, 'v'));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, true, 'v');
+
+  v.push_back(std::make_tuple(2ULL, 1, 0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(2ULL, 1, 0);
+
+  v.push_back(std::make_tuple<long long, int, int>(3LL, 1, 0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_tuple<long long, int, int>(3LL, 1, 0));
+  // make_tuple is not removed when there are explicit template
+  // arguments provided.
+}
+
+namespace test {
+template <typename T> struct Single {
+  Single() = default;
+  Single(const Single &) = default;
+  Single(Single &&) = default;
+
+  Single(const T &) {}
+  Single(T &&) {}
+
+  template <typename U> Single(const Single<U> &) {}
+  template <typename U> Single(Single<U> &&) {}
+
+  template <typename U> Single(const std::tuple<U> &) {}
+  template <typename U> Single(std::tuple<U> &&) {}
+};
+
+template <typename T>
+Single<typename std::remove_reference<T>::type> MakeSingle(T &&) {
+  return {};
+}
+} // namespace test
+
+void testOtherTuples() {
+  std::vector<test::Single<int>> v;
+  v.push_back(test::Single<int>(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1);
+
+  v.push_back({2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(2);
+
+  v.push_back(test::MakeSingle(3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(3);
+
+  v.push_back(test::MakeSingle<long long>(4));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(test::MakeSingle<long long>(4));
+  // We don't remove make functions with explicit template parameters.
+
+  v.push_back(test::MakeSingle(5LL));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(5LL);
+
+  v.push_back(std::make_tuple(6));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(6);
+
+  v.push_back(std::make_tuple(7LL));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(7LL);
+}
+
 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);
 
   std::deque<Something> d;
   d.push_back(Something(42));

 
 class PrivateCtor {
   PrivateCtor(int z);
 
 public:
   void doStuff() {
     std::vector<PrivateCtor> v;
     // This should not change it because emplace back doesn't have permission.
-    // Check currently doesn't support friend delcarations because pretty much
+    // Check currently doesn't support friend declarations because pretty much
     // nobody would want to be friend with std::vector :(.
     v.push_back(PrivateCtor(42));
   }
 };
 
 struct WithDtor {
   WithDtor(int) {}
   ~WithDtor();