fix handling of braced-init temporaries for modernize-use-emplace

[BigPeet]

Summary

Addresses https://github.com/llvm/llvm-project/issues/55870

[BigPeet]

• update comments

[BigPeet]

applying clang-format

[BigPeet]

fixed wrong arc diff handling :-/

[BigPeet]

Just a reminder. Do I have to do anything?

[BigPeet]

Ping

[BigPeet]

Ping

[njames93]

clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp

1268

Is this a test case that wasn't already caught, as braces aren't being used here

[BigPeet]

Thanks for the review. Do I have to do anything or how will this be handled from here?

clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp

1268

This case was/is already caught. I added this check (and a few others) to make sure I don't break that behavior.
It might be redundant and could be removed.

[BigPeet]

Ping.

(Sorry, it's my first time contributing to LLVM and I simply don't know what happens next. Do I need to do anything? Or is it just waiting to get merged at some point?)

[njames93]

In D135405#4039820, @BigPeet wrote:

Ping.

(Sorry, it's my first time contributing to LLVM and I simply don't know what happens next. Do I need to do anything? Or is it just waiting to get merged at some point?)

My apologies, what is your GitHub username and email so I can attribute you as the author of this patch.

[BigPeet]

GitHub username: BigPeet
GitHub email: [e-mail]

Thanks for your quick response.

[BigPeet]

Ping?

clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp

auto IsCtorOfSmartPtr =
       hasDeclaration(cxxConstructorDecl(ofClass(hasAnyName(SmartPointers))));
 
   // Bitfields binds only to consts and emplace_back take it by universal ref.
   auto BitFieldAsArgument = hasAnyArgument(
       ignoringImplicit(memberExpr(hasDeclaration(fieldDecl(isBitField())))));
 
   // Initializer list can't be passed to universal reference.
   auto InitializerListAsArgument = hasAnyArgument(
-      ignoringImplicit(cxxConstructExpr(isListInitialization())));
+      ignoringImplicit(allOf(cxxConstructExpr(isListInitialization()),
+                             unless(cxxTemporaryObjectExpr()))));
 
   // We could have leak of resource.
   auto NewExprAsArgument = hasAnyArgument(ignoringImplicit(cxxNewExpr()));
   // We would call another constructor.
   auto ConstructingDerived =
       hasParent(implicitCastExpr(hasCastKind(CastKind::CK_DerivedToBase)));
 
   // emplace_back can't access private or protected constructors.
   auto IsPrivateOrProtectedCtor =
       hasDeclaration(cxxConstructorDecl(anyOf(isPrivate(), isProtected())));
 
   auto HasInitList = anyOf(has(ignoringImplicit(initListExpr())),
                            has(cxxStdInitializerListExpr()));
 
   // FIXME: Discard 0/NULL (as nullptr), static inline const data members,
   // overloaded functions and template names.
   auto SoughtConstructExpr =
       cxxConstructExpr(
           unless(anyOf(IsCtorOfSmartPtr, HasInitList, BitFieldAsArgument,
                        InitializerListAsArgument, NewExprAsArgument,
                        ConstructingDerived, IsPrivateOrProtectedCtor)))
           .bind("ctor");
   auto HasConstructExpr = has(ignoringImplicit(SoughtConstructExpr));
 
+  // allow for T{} to be replaced, even if no CTOR is declared
+  auto HasConstructInitListExpr = has(initListExpr(anyOf(
+      allOf(has(SoughtConstructExpr),
+            has(cxxConstructExpr(argumentCountIs(0)))),
+      has(cxxBindTemporaryExpr(has(SoughtConstructExpr),
+                               has(cxxConstructExpr(argumentCountIs(0))))))));
+  auto HasBracedInitListExpr =
+      anyOf(has(cxxBindTemporaryExpr(HasConstructInitListExpr)),
+            HasConstructInitListExpr);
+
   auto MakeTuple = ignoringImplicit(
       callExpr(callee(expr(ignoringImplicit(declRefExpr(
                    unless(hasExplicitTemplateArgs()),
                    to(functionDecl(hasAnyName(TupleMakeFunctions))))))))
           .bind("make"));
 
   // make_something can return type convertible to container's element type.
   // Allow the conversion only on containers of pairs.
   auto MakeTupleCtor = ignoringImplicit(cxxConstructExpr(
       has(materializeTemporaryExpr(MakeTuple)),
       hasDeclaration(cxxConstructorDecl(ofClass(hasAnyName(TupleTypes))))));
 
-  auto SoughtParam = materializeTemporaryExpr(
+  auto SoughtParam =
+      materializeTemporaryExpr(
           anyOf(has(MakeTuple), has(MakeTupleCtor), HasConstructExpr,
-            has(cxxFunctionalCastExpr(HasConstructExpr))));
+                HasBracedInitListExpr,
+                has(cxxFunctionalCastExpr(HasConstructExpr)),
+                has(cxxFunctionalCastExpr(HasBracedInitListExpr))))
+          .bind("temporary_expr");
 
   auto HasConstructExprWithValueTypeType =
       has(ignoringImplicit(cxxConstructExpr(
           SoughtConstructExpr, hasType(type(hasUnqualifiedDesugaredType(
                                    type(equalsBoundNode("value_type"))))))));
 
-  auto HasConstructExprWithValueTypeTypeAsLastArgument =
-      hasLastArgument(materializeTemporaryExpr(anyOf(
-          HasConstructExprWithValueTypeType,
-          has(cxxFunctionalCastExpr(HasConstructExprWithValueTypeType)))));
+  auto HasBracedInitListWithValueTypeType =
+      anyOf(allOf(HasConstructInitListExpr,
+                  has(initListExpr(hasType(type(hasUnqualifiedDesugaredType(
+                      type(equalsBoundNode("value_type")))))))),
+            has(cxxBindTemporaryExpr(
+                HasConstructInitListExpr,
+                has(initListExpr(hasType(type(hasUnqualifiedDesugaredType(
+                    type(equalsBoundNode("value_type"))))))))));
+
+  auto HasConstructExprWithValueTypeTypeAsLastArgument = hasLastArgument(
+      materializeTemporaryExpr(
+          anyOf(HasConstructExprWithValueTypeType,
+                HasBracedInitListWithValueTypeType,
+                has(cxxFunctionalCastExpr(HasConstructExprWithValueTypeType)),
+                has(cxxFunctionalCastExpr(HasBracedInitListWithValueTypeType))))
+          .bind("temporary_expr"));
 
   Finder->addMatcher(
       traverse(TK_AsIs, cxxMemberCallExpr(CallPushBack, has(SoughtParam),
                                           unless(isInTemplateInstantiation()))
                             .bind("push_back_call")),
       this);
 
   Finder->addMatcher(
const auto *PushBackCall =
       Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_back_call");
   const auto *PushCall = Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_call");
   const auto *PushFrontCall =
       Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_front_call");
   const auto *EmplacyCall =
       Result.Nodes.getNodeAs<CXXMemberCallExpr>("emplacy_call");
   const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctor");
   const auto *MakeCall = Result.Nodes.getNodeAs<CallExpr>("make");
+  const auto *TemporaryExpr =
+      Result.Nodes.getNodeAs<MaterializeTemporaryExpr>("temporary_expr");
 
   const CXXMemberCallExpr *Call = [&]() {
     if (PushBackCall) {
       return PushBackCall;
     }
     if (PushCall) {
       return PushCall;
     }
if (IgnoreImplicitConstructors && CtorCall && CtorCall->getNumArgs() >= 1 &&
       CtorCall->getArg(0)->getSourceRange() == CtorCall->getSourceRange())
     return;
 
   const auto FunctionNameSourceRange = CharSourceRange::getCharRange(
       Call->getExprLoc(), Call->getArg(0)->getExprLoc());
 
   auto Diag =
       EmplacyCall
-          ? diag(CtorCall ? CtorCall->getBeginLoc() : MakeCall->getBeginLoc(),
+          ? diag(TemporaryExpr ? TemporaryExpr->getBeginLoc()
+                 : CtorCall    ? CtorCall->getBeginLoc()
+                               : MakeCall->getBeginLoc(),
                  "unnecessary temporary object created while calling %0")
           : diag(Call->getExprLoc(), "use emplace%select{|_back|_front}0 "
                                      "instead of push%select{|_back|_front}0");
   if (EmplacyCall)
     Diag << Call->getMethodDecl()->getName();
   else if (PushCall)
     Diag << 0;
   else if (PushBackCall)
const SourceRange CallParensRange =
       MakeCall ? SourceRange(MakeCall->getCallee()->getEndLoc(),
                              MakeCall->getRParenLoc())
                : CtorCall->getParenOrBraceRange();
 
   // Finish if there is no explicit constructor call.
   if (CallParensRange.getBegin().isInvalid())
     return;
 
-  const SourceLocation ExprBegin =
-      CtorCall ? CtorCall->getExprLoc() : MakeCall->getExprLoc();
+  // FIXME: Will there ever be a CtorCall, if there is no TemporaryExpr?
+  const SourceLocation ExprBegin = TemporaryExpr ? TemporaryExpr->getExprLoc()
+                                   : CtorCall    ? CtorCall->getExprLoc()
+                                                 : MakeCall->getExprLoc();
 
   // Range for constructor name and opening brace.
   const auto ParamCallSourceRange =
       CharSourceRange::getTokenRange(ExprBegin, CallParensRange.getBegin());
 
+  // Range for constructor closing brace and end of temporary expr.
+  const auto EndCallSourceRange = CharSourceRange::getTokenRange(
+      CallParensRange.getEnd(),
+      TemporaryExpr ? TemporaryExpr->getEndLoc() : CallParensRange.getEnd());
+
   Diag << FixItHint::CreateRemoval(ParamCallSourceRange)
-       << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
-              CallParensRange.getEnd(), CallParensRange.getEnd()));
+       << FixItHint::CreateRemoval(EndCallSourceRange);
 
   if (MakeCall && EmplacyCall) {
     // Remove extra left parenthesis
     Diag << FixItHint::CreateRemoval(
         CharSourceRange::getCharRange(MakeCall->getCallee()->getEndLoc(),
                                       MakeCall->getArg(0)->getBeginLoc()));
   }
 }

clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp

void testPossibleFalsePositives() {
   v.emplace_back(std::make_pair<char, char>(0, 3));
 
   struct D {
     D(...) {}
     operator char() const { return 0; }
   };
   v.emplace_back(std::make_pair<D, int>(Something(), 2));
 }
+
+struct InnerType {
+  InnerType();
+  InnerType(char const*);
+};
+
+struct NonTrivialNoCtor {
+  InnerType it;
+};
+
+struct NonTrivialWithVector {
+  std::vector<int> it;
+};
+
+struct NonTrivialWithCtor {
+  NonTrivialWithCtor();
+  NonTrivialWithCtor(std::vector<int> const&);
+};
+
+void testBracedInitTemporaries() {
+  std::vector<NonTrivialNoCtor> v1;
+
+  v1.push_back(NonTrivialNoCtor());
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v1.emplace_back();
+  v1.push_back(NonTrivialNoCtor{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v1.emplace_back();
+  v1.push_back({});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v1.emplace_back();
+  v1.push_back(NonTrivialNoCtor{InnerType{}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v1.emplace_back();
+  v1.push_back({InnerType{}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v1.emplace_back();
+  v1.push_back(NonTrivialNoCtor{InnerType()});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v1.emplace_back();
+  v1.push_back({InnerType()});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v1.emplace_back();
+  v1.push_back({{}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v1.emplace_back();
+
+  v1.emplace_back(NonTrivialNoCtor());
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: v1.emplace_back();
+  v1.emplace_back(NonTrivialNoCtor{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: v1.emplace_back();
+  v1.emplace_back(NonTrivialNoCtor{InnerType{}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: v1.emplace_back();
+  v1.emplace_back(NonTrivialNoCtor{{}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: v1.emplace_back();
+
+  // These should not be noticed or fixed; after the correction, the code won't
+  // compile.
+  v1.push_back(NonTrivialNoCtor{""});
+  v1.push_back({""});
+  v1.push_back(NonTrivialNoCtor{InnerType{""}});
+  v1.push_back({InnerType{""}});
+  v1.emplace_back(NonTrivialNoCtor{""});
+
+  std::vector<NonTrivialWithVector> v2;
+
+  v2.push_back(NonTrivialWithVector());
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v2.emplace_back();
+  v2.push_back(NonTrivialWithVector{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v2.emplace_back();
+  v2.push_back({});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v2.emplace_back();
+  v2.push_back(NonTrivialWithVector{std::vector<int>{}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v2.emplace_back();
+  v2.push_back({std::vector<int>{}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v2.emplace_back();
+  v2.push_back(NonTrivialWithVector{std::vector<int>()});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v2.emplace_back();
+  v2.push_back({std::vector<int>()});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v2.emplace_back();
+  v2.push_back({{}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v2.emplace_back();
+
+  v2.emplace_back(NonTrivialWithVector());
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: v2.emplace_back();
+  v2.emplace_back(NonTrivialWithVector{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: v2.emplace_back();
+  v2.emplace_back(NonTrivialWithVector{std::vector<int>{}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: v2.emplace_back();
+  v2.emplace_back(NonTrivialWithVector{{}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: v2.emplace_back();
+
+
+  // These should not be noticed or fixed; after the correction, the code won't
+  // compile.
+  v2.push_back(NonTrivialWithVector{{0}});
+  v2.push_back({{0}});
+  v2.push_back(NonTrivialWithVector{std::vector<int>{0}});
+  v2.push_back({std::vector<int>{0}});
+  v2.emplace_back(NonTrivialWithVector{std::vector<int>{0}});
+
+  std::vector<NonTrivialWithCtor> v3;
+
+  v3.push_back(NonTrivialWithCtor());
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v3.emplace_back();
+  v3.push_back(NonTrivialWithCtor{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v3.emplace_back();
+  v3.push_back({});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v3.emplace_back();
+  v3.push_back(NonTrivialWithCtor{std::vector<int>()});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v3.emplace_back(std::vector<int>());
+  v3.push_back(NonTrivialWithCtor{std::vector<int>{0}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v3.emplace_back(std::vector<int>{0});
+  v3.push_back(NonTrivialWithCtor{std::vector<int>{}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v3.emplace_back(std::vector<int>{});
+  v3.push_back({std::vector<int>{0}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v3.emplace_back(std::vector<int>{0});
+  v3.push_back({std::vector<int>{}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: v3.emplace_back(std::vector<int>{});
+
+  v3.emplace_back(NonTrivialWithCtor());
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: v3.emplace_back();
+  v3.emplace_back(NonTrivialWithCtor{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: v3.emplace_back();
+  v3.emplace_back(NonTrivialWithCtor{std::vector<int>{}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: v3.emplace_back(std::vector<int>{});
+  v3.emplace_back(NonTrivialWithCtor{std::vector<int>{0}});
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: v3.emplace_back(std::vector<int>{0});
+
+  // These should not be noticed or fixed; after the correction, the code won't
+  // compile.
+  v3.push_back(NonTrivialWithCtor{{0}});
+  v3.push_back(NonTrivialWithCtor{{}});
+  v3.push_back({{0}});
+  v3.push_back({{}});
+}