kuhar updated this revision to Diff 96332.
kuhar marked an inline comment as done.
kuhar added a comment.

Update modernize-use-emplace rst docs.


https://reviews.llvm.org/D32395

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===================================================================
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -53,8 +53,8 @@
 };
 
 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,42 @@
 
 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 example below illustrates 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.
 }
 
-void testOtherCointainers() {
+void testOtherContainers() {
   std::list<Something> l;
   l.push_back(Something(42, 41));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
Index: docs/clang-tidy/checks/modernize-use-emplace.rst
===================================================================
--- docs/clang-tidy/checks/modernize-use-emplace.rst
+++ docs/clang-tidy/checks/modernize-use-emplace.rst
@@ -36,8 +36,7 @@
 
     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.
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -101,6 +101,12 @@
   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
 -----------------------------
 
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===================================================================
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -15,6 +15,12 @@
 namespace tidy {
 namespace modernize {
 
+namespace {
+AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
+  return Node.hasExplicitTemplateArgs();
+}
+} // namespace
+
 static const auto DefaultContainersWithPushBack =
     "::std::vector; ::std::list; ::std::deque";
 static const auto DefaultSmartPointers =
@@ -80,18 +86,31 @@
           .bind("ctor");
   auto hasConstructExpr = has(ignoringImplicit(soughtConstructExpr));
 
-  auto ctorAsArgument = materializeTemporaryExpr(
-      anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr))));
+  auto makePair = ignoringImplicit(
+      callExpr(callee(expr(ignoringParenImpCasts(
+          declRefExpr(unless(hasExplicitTemplateArgs()),
+                      to(functionDecl(hasName("::std::make_pair"))))
+      )))).bind("make_pair"));
+
+  // make_pair can return type convertible to container's element type.
+  auto makePairCtor = ignoringImplicit(cxxConstructExpr(
+      has(materializeTemporaryExpr(makePair))));
 
-  Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(ctorAsArgument),
+  auto soughtParam = materializeTemporaryExpr(
+      anyOf(has(makePair), has(makePairCtor),
+            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);
 
   auto FunctionNameSourceRange = CharSourceRange::getCharRange(
       Call->getExprLoc(), Call->getArg(0)->getExprLoc());
@@ -101,22 +120,34 @@
   if (FunctionNameSourceRange.getBegin().isMacroID())
     return;
 
+  const auto *EmplacePrefix = MakePairCall ? "emplace_back" : "emplace_back(";
   Diag << FixItHint::CreateReplacement(FunctionNameSourceRange,
-                                       "emplace_back(");
+                                       EmplacePrefix);
 
-  auto CallParensRange = InnerCtorCall->getParenOrBraceRange();
+  SourceRange CallParensRange;
+  if (MakePairCall)
+    CallParensRange = SourceRange(MakePairCall->getCallee()->getLocEnd(),
+                                  MakePairCall->getRParenLoc());
+  else
+    CallParensRange = InnerCtorCall->getParenOrBraceRange();
 
   // Finish if there is no explicit constructor call.
   if (CallParensRange.getBegin().isInvalid())
     return;
 
+  SourceLocation ExprBegin;
+  if (MakePairCall)
+    ExprBegin = MakePairCall->getExprLoc();
+  else
+    ExprBegin = InnerCtorCall->getExprLoc();
+
   // Range for constructor name and opening brace.
-  auto CtorCallSourceRange = CharSourceRange::getTokenRange(
-      InnerCtorCall->getExprLoc(), CallParensRange.getBegin());
+  auto ParamCallSourceRange = CharSourceRange::getTokenRange(
+      ExprBegin, CallParensRange.getBegin());
 
-  Diag << FixItHint::CreateRemoval(CtorCallSourceRange)
+  Diag << FixItHint::CreateRemoval(ParamCallSourceRange)
        << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
-              CallParensRange.getEnd(), CallParensRange.getEnd()));
+           CallParensRange.getEnd(), CallParensRange.getEnd()));
 }
 
 void UseEmplaceCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to