diff options
4 files changed, 88 insertions, 24 deletions
diff --git a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp index 4084454ca0d..4c6991ccc16 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp +++ b/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")); + + // 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")))))); - 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,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; + 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) { diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index a4db0b6cf4f..40ef36d8079 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/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 ----------------------------- diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize-use-emplace.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize-use-emplace.rst index e6beddd9ed4..53fb7a7be75 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/modernize-use-emplace.rst +++ b/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. diff --git a/clang-tools-extra/test/clang-tidy/modernize-use-emplace.cpp b/clang-tools-extra/test/clang-tidy/modernize-use-emplace.cpp index a82a3a5ba0d..151e9f3fec0 100644 --- a/clang-tools-extra/test/clang-tidy/modernize-use-emplace.cpp +++ b/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 |
