Index: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp @@ -15,10 +15,16 @@ 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 @@ // 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( @@ -80,10 +85,23 @@ .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); @@ -92,8 +110,10 @@ 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) && "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 @@ 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())); + CallParensRange.getEnd(), CallParensRange.getEnd())); } void UseEmplaceCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Index: clang-tools-extra/trunk/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/trunk/docs/ReleaseNotes.rst +++ clang-tools-extra/trunk/docs/ReleaseNotes.rst @@ -106,6 +106,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: clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst +++ clang-tools-extra/trunk/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: clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp +++ clang-tools-extra/trunk/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,51 @@ 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 examples below illustrate 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. + + struct Y { + Y(std::pair&&) {} + }; + std::vector 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 l; l.push_back(Something(42, 41)); // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back