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,10 +86,21 @@ .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); @@ -92,6 +109,8 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { const auto *Call = Result.Nodes.getNodeAs("call"); const auto *InnerCtorCall = Result.Nodes.getNodeAs("ctor"); + const auto *MakePairCall = Result.Nodes.getNodeAs("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) { 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 + `_ 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: 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> 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: 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 -pair make_pair(T1, T2) { - return pair(); +pair make_pair(T1&&, T2&&) { + return {}; }; template @@ -274,18 +274,42 @@ void testMakePair() { std::vector> 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(0, 3)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(std::make_pair(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(Something(), 2)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(std::make_pair(Something(), 2)); + + struct X { + X(std::pair) {} + }; + std::vector 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 l; l.push_back(Something(42, 41)); // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back