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

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.

Reviewers: Prazek, aaron.ballman, hokein, alexfh

Reviewed By: Prazek, alexfh

Subscribers: JDevlieghere, JonasToth, cfe-commits

Tags: #clang-tools-extra

Differential Revision: https://reviews.llvm.org/D32395

llvm-svn: 301651

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

@@ -15,10 +15,16 @@ 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),
@@ -39,7 +45,6 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
   // 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>(
@@ -80,10 +85,23 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
           .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"));
 
-  Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(ctorAsArgument),
+  // 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(soughtParam),
                                        unless(isInTemplateInstantiation()))
                          .bind("call"),
                      this);
@@ -92,8 +110,10 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
 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");
@@ -101,22 +121,28 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
   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;
 
-  // Range for constructor name and opening brace.
-  auto CtorCallSourceRange = CharSourceRange::getTokenRange(
-      InnerCtorCall->getExprLoc(), CallParensRange.getBegin());
+  const SourceLocation ExprBegin =
+      MakePairCall ? MakePairCall->getExprLoc() : InnerCtorCall->getExprLoc();
 
-  Diag << FixItHint::CreateRemoval(CtorCallSourceRange)
+  // 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()));
+           CallParensRange.getEnd(), CallParensRange.getEnd()));
 }
 
 void UseEmplaceCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {

clang-tools-extra/docs/ReleaseNotes.rst

@@ -106,6 +106,12 @@ Improvements to clang-tidy
   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
 -----------------------------
 

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

@@ -36,8 +36,7 @@ After:
 
     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.

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

@@ -53,8 +53,8 @@ public:
 };
 
 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>
@@ -274,18 +274,51 @@ void testPointers() {
 
 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