kuhar updated this revision to Diff 96622.
kuhar marked 4 inline comments as done.
kuhar added a comment.
Added const where possible, moved from if-else to ternary.
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,51 @@
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
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,43 +86,64 @@
.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.
+ // 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);
}
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");
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) {
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
