[clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

[kuhar]

Summary

When there is a push_back with a call to make_pair, turn it into emplace_back and remove the unnecessary make_pair call.

Eg.

std::vector<std::pair<int, int>> v;
v.push_back(std::make_pair(1, 2)); // --> v.emplace_back(1, 2);

make_pair doesn't get removed when explicit template parameters are provided, because of potential problems with type conversions.

[JonasToth]

please note this enhancement in the ReleaseNotes.

clang-tidy/modernize/UseEmplaceCheck.cpp

93 ↗

(On Diff #96298)

is the new line here necessary? i think it looks better if the .bind is on this line.

97 ↗

(On Diff #96298)

here, on line 100 and 89: shouldnt the matchers be upper case since they are variables? Iam unsure about that.

[Prazek]

clang-tidy/modernize/UseEmplaceCheck.cpp

93 ↗

(On Diff #96298)

Better question is "is it clang formated?"

97 ↗

(On Diff #96298)

True, all the matchers should have upper case, but it would be better to send it in separate patch

[Prazek]

test/clang-tidy/modernize-use-emplace.cpp

284 ↗

(On Diff #96298)

I would add here test like:

class X {
  X(std:;pair<int, int> a) {}
};

std::vector<X> v;
v.push_back(make_pair(42, 42));

I guess as long as X ctor is not explicit this can happen, and we can't transform it to
emplace.back(42, 42)

[kuhar]

Add test for pair constructor argument, mention changes in release notes.

[kuhar]

clang-tidy/modernize/UseEmplaceCheck.cpp

93 ↗

(On Diff #96298)

It was clang-formatted, but I do agree that it looked a bit weird. I removed the extra newline.

test/clang-tidy/modernize-use-emplace.cpp

284 ↗

(On Diff #96298)

Nice idea for a test case, added.

[kuhar]

Update modernize-use-emplace rst docs.

[kuhar]

test/clang-tidy/modernize-use-emplace.cpp

284 ↗

(On Diff #96298)

A better test case would be to make X's ctor take a pair by const reference or rvalue reference. This way it wouldn't produce an extra CxxConstructExpr and will be currently (incorrectly) matched. This needs to be fixed.

[kuhar]

Don't remove make_pair calls when inserted type is different than std::pair.

[JDevlieghere]

clang-tidy/modernize/UseEmplaceCheck.cpp

115 ↗

(On Diff #96384)

It's highly recommended to put some kind of error message in the assertion statement, as per the LLVM coding standards.

130 ↗

(On Diff #96384)

Can't we use the ternary operator here?

140 ↗

(On Diff #96384)

Same here?

147 ↗

(On Diff #96384)

I think this can be const.

[kuhar]

Added const where possible, moved from if-else to ternary.

[kuhar]

Thanks for the suggestions, Jonas. Fixed.

[Prazek]

clang-tidy/modernize/UseEmplaceCheck.cpp

22 ↗

(On Diff #96622)

extend the anonymous namespace to have static strings (and remove static)

90 ↗

(On Diff #96622)

Why ignoringParenImpCasts instead of ingnoringImplicit?

115 ↗

(On Diff #96384)

would it be better to change it to
!innerCtorCall && !MakePairCall && "No .."

[kuhar]

Use igoringImplicit instead of ignoringParenImpCasts. Options moved to the anonymous namespace.

[kuhar]

clang-tidy/modernize/UseEmplaceCheck.cpp

115 ↗

(On Diff #96384)

I don't think that this logic would work here. !first && !second ensures both are null.

[Prazek]

clang-tidy/modernize/UseEmplaceCheck.cpp

115 ↗

(On Diff #96384)

Yea, I forgot how to De Morgan

[kuhar]

I run clang-tidy with this version of modernize-use-emplace on the whole llvm + clang + clang tools extra tree and it emitted ~500 fixits, ~100 of them were make_pair ones.
The whole codebase compiled fine and there were no new test failures with check-all.

[Prazek]

LGTM

[alexfh]

LG

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

 #include "UseEmplaceCheck.h"
 #include "../utils/OptionsUtils.h"
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
-static const auto DefaultContainersWithPushBack =
+namespace {
+AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
+  return Node.hasExplicitTemplateArgs();
+}
+
+const auto DefaultContainersWithPushBack =
     "::std::vector; ::std::list; ::std::deque";
-static const auto DefaultSmartPointers =
+const auto DefaultSmartPointers =
     "::std::shared_ptr; ::std::unique_ptr; ::std::auto_ptr; ::std::weak_ptr";
+} // 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))) {}
 
 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.
   // + 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
-  // + match for make_pair calls.
   auto callPushBack = cxxMemberCallExpr(
       hasDeclaration(functionDecl(hasName("push_back"))),
       on(hasType(cxxRecordDecl(hasAnyName(SmallVector<StringRef, 5>(
           ContainersWithPushBack.begin(), ContainersWithPushBack.end()))))));
 
   // We can't replace push_backs of smart pointer because
   // if emplacement fails (f.e. bad_alloc in vector) we will have leak of
   // passed pointer because smart pointer won't be constructed
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 ctorAsArgument = materializeTemporaryExpr(
-      anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr))));
+  auto makePair = ignoringImplicit(
+      callExpr(callee(expr(ignoringImplicit(
+          declRefExpr(unless(hasExplicitTemplateArgs()),
+                      to(functionDecl(hasName("::std::make_pair"))))
+      )))).bind("make_pair"));
+
+  // make_pair 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 soughtParam = materializeTemporaryExpr(
+      anyOf(has(makePair), has(makePairCtor),
+            hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr))));
 
-  Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(ctorAsArgument),
+  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");
 
-  auto FunctionNameSourceRange = CharSourceRange::getCharRange(
+  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;
 
-  Diag << FixItHint::CreateReplacement(FunctionNameSourceRange,
-                                       "emplace_back(");
+  const auto *EmplacePrefix = MakePairCall ? "emplace_back" : "emplace_back(";
+  Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, EmplacePrefix);
 
-  auto CallParensRange = InnerCtorCall->getParenOrBraceRange();
+  const SourceRange CallParensRange =
+      MakePairCall ? SourceRange(MakePairCall->getCallee()->getLocEnd(),
+                                 MakePairCall->getRParenLoc())
+                   : InnerCtorCall->getParenOrBraceRange();
 
   // Finish if there is no explicit constructor call.
   if (CallParensRange.getBegin().isInvalid())
     return;
 
+  const SourceLocation ExprBegin =
+      MakePairCall ? MakePairCall->getExprLoc() : InnerCtorCall->getExprLoc();
+
   // Range for constructor name and opening brace.
-  auto CtorCallSourceRange = CharSourceRange::getTokenRange(
-      InnerCtorCall->getExprLoc(), CallParensRange.getBegin());
+  const auto ParamCallSourceRange =
+      CharSourceRange::getTokenRange(ExprBegin, CallParensRange.getBegin());
 
-  Diag << FixItHint::CreateRemoval(CtorCallSourceRange)
+  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));
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

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

- Support clang-formatting of the code around applied fixes (``-format-style``
   command-line option).
 
 - 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.
 
+- 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).
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

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);
-    // This will be fixed to w.emplace_back(21L, 37L); in next version
-    w.emplace_back(std::make_pair(21L, 37L);
+    w.emplace_back(21L, 37L);
 
 The other situation is when we pass arguments that will be converted to a type
 inside a container.
 
 Before:
 
 .. code-block:: c++
 

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

public:
 
   template <class U1, class U2>
   pair(const pair<U1, U2> &p){};
   template <class U1, class U2>
   pair(pair<U1, U2> &&p){};
 };
 
 template <typename T1, typename T2>
-pair<T1, T2> make_pair(T1, T2) {
-  return pair<T1, T2>();
+pair<T1, T2> make_pair(T1&&, T2&&) {
+  return {};
 };
 
 template <typename T>
 class unique_ptr {
 public:
   explicit unique_ptr(T *) {}
   ~unique_ptr();
 };
void testPointers() {
 
   auto *ptr = new int;
   v2.push_back(std::unique_ptr<int>(ptr));
   // Same here
 }
 
 void testMakePair() {
   std::vector<std::pair<int, int>> v;
-  // FIXME: add functionality to change calls of std::make_pair
   v.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
 
-  // FIXME: This is not a bug, but call of make_pair should be removed in the
-  // future. This one matches because the return type of make_pair is different
-  // than the pair itself.
   v.push_back(std::make_pair(42LL, 13));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
-  // CHECK-FIXES: v.emplace_back(std::make_pair(42LL, 13));
+  // CHECK-FIXES: v.emplace_back(42LL, 13);
+
+  v.push_back(std::make_pair<char, char>(0, 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair<char, char>(0, 3));
+  //
+  // Even though the call above could be turned into v.emplace_back(0, 3),
+  // we don't eliminate the make_pair call here, because of the explicit
+  // template parameters provided. make_pair's arguments can be convertible
+  // to its explicitly provided template parameter, but not to the pair's
+  // element type. The examples below illustrate the problem.
+  struct D {
+    D(...) {}
+    operator char() const { return 0; }
+  };
+  v.push_back(std::make_pair<D, int>(Something(), 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair<D, int>(Something(), 2));
+
+  struct X {
+    X(std::pair<int, int>) {}
+  };
+  std::vector<X> x;
+  x.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(std::make_pair(1, 2));
+  // make_pair cannot be removed here, as X is not constructible with two ints.
+
+  struct Y {
+    Y(std::pair<int, int>&&) {}
+  };
+  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 testOtherCointainers() {
+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));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back