[clang-tools-extra] r301651 - [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls
Jakub Kuderski via cfe-commits
cfe-commits at lists.llvm.org
Fri Apr 28 09:25:45 PDT 2017
Author: kuhar
Date: Fri Apr 28 11:25:45 2017
New Revision: 301651
URL: http://llvm.org/viewvc/llvm-project?rev=301651&view=rev
Log:
[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
Modified:
clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst
clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp
Modified: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp?rev=301651&r1=301650&r2=301651&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp Fri Apr 28 11:25:45 2017
@@ -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(M
// 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(M
.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::registerMatchers(M
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 MatchF
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) {
Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=301651&r1=301650&r2=301651&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Fri Apr 28 11:25:45 2017
@@ -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
-----------------------------
Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst?rev=301651&r1=301650&r2=301651&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst Fri Apr 28 11:25:45 2017
@@ -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.
Modified: clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp?rev=301651&r1=301650&r2=301651&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp Fri Apr 28 11:25:45 2017
@@ -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
More information about the cfe-commits
mailing list