[clang-tidy] Add support for Initialization Forwarding in Nested Objects within modernize-use-emplace

Author: RiverDave

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

@@ -98,8 +98,8 @@ auto hasWantedType(llvm::ArrayRef<StringRef> TypeNames) {
 
 // Matches member call expressions of the named method on the listed container
 // types.
-auto cxxMemberCallExprOnContainer(
-    StringRef MethodName, llvm::ArrayRef<StringRef> ContainerNames) {
+auto cxxMemberCallExprOnContainer(StringRef MethodName,
+                                  llvm::ArrayRef<StringRef> ContainerNames) {
   return cxxMemberCallExpr(
       hasDeclaration(functionDecl(hasName(MethodName))),
       on(hasTypeOrPointeeType(hasWantedType(ContainerNames))));
@@ -174,19 +174,19 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
   // passed pointer because smart pointer won't be constructed
   // (and destructed) as in push_back case.
   auto IsCtorOfSmartPtr =
-      hasDeclaration(cxxConstructorDecl(ofClass(hasAnyName(SmartPointers))));
+      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())))));
+  auto BitFieldAsArgument =
+      ignoringImplicit(memberExpr(hasDeclaration(fieldDecl(isBitField()))));
 
   // Initializer list can't be passed to universal reference.
-  auto InitializerListAsArgument = hasAnyArgument(
+  auto InitializerListAsArgument =
       ignoringImplicit(allOf(cxxConstructExpr(isListInitialization()),
-                             unless(cxxTemporaryObjectExpr()))));
+                             unless(cxxTemporaryObjectExpr())));
 
   // We could have leak of resource.
-  auto NewExprAsArgument = hasAnyArgument(ignoringImplicit(cxxNewExpr()));
+  auto NewExprAsArgument = ignoringImplicit(cxxNewExpr());
   // We would call another constructor.
   auto ConstructingDerived =
       hasParent(implicitCastExpr(hasCastKind(CastKind::CK_DerivedToBase)));
@@ -202,11 +202,26 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
   // overloaded functions and template names.
   auto SoughtConstructExpr =
       cxxConstructExpr(
-          unless(anyOf(IsCtorOfSmartPtr, HasInitList, BitFieldAsArgument,
-                       InitializerListAsArgument, NewExprAsArgument,
-                       ConstructingDerived, IsPrivateOrProtectedCtor)))
+          unless(anyOf(hasDeclaration(IsCtorOfSmartPtr), HasInitList,
+                       hasAnyArgument(BitFieldAsArgument),
+                       hasAnyArgument(InitializerListAsArgument),
+                       hasAnyArgument(NewExprAsArgument), ConstructingDerived,
+                       IsPrivateOrProtectedCtor)))
           .bind("ctor");
-  auto HasConstructExpr = has(ignoringImplicit(SoughtConstructExpr));
+
+  auto IsPrimitiveType = hasType(builtinType());
+
+  auto AggregateInitExpr =
+      getLangOpts().CPlusPlus20
+          ? initListExpr(unless(anyOf(HasInitList, has(IsCtorOfSmartPtr),
+                                      has(BitFieldAsArgument),
+                                      has(InitializerListAsArgument),
+                                      has(NewExprAsArgument), IsPrimitiveType)))
+                .bind("agg_init")
+          : unless(anything());
+
+  auto HasConstructExpr =
+      has(ignoringImplicit(anyOf(SoughtConstructExpr, AggregateInitExpr)));
 
   // allow for T{} to be replaced, even if no CTOR is declared
   auto HasConstructInitListExpr = has(initListExpr(
@@ -305,6 +320,34 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
       this);
 }
 
+static const CXXConstructExpr *unwrapToConstructorExpr(const Expr *E) {
+  while (E) {
+    if (const auto *ConstructorExpr = llvm::dyn_cast<CXXConstructExpr>(E)) {
+      return ConstructorExpr;
+    }
+
+    if (const auto *BindTemp = llvm::dyn_cast<CXXBindTemporaryExpr>(E)) {
+      E = BindTemp->getSubExpr();
+      continue;
+    }
+
+    if (const auto *MaterialTemp =
+            llvm::dyn_cast<MaterializeTemporaryExpr>(E)) {
+      E = MaterialTemp->getSubExpr();
+      continue;
+    }
+
+    if (const auto *Cast = llvm::dyn_cast<ImplicitCastExpr>(E)) {
+      E = Cast->getSubExpr();
+      continue;
+    }
+
+    break;
+  }
+
+  return nullptr; // No relevant sub-expression found
+}
+
 void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *PushBackCall =
       Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_back_call");
@@ -313,7 +356,8 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
       Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_front_call");
   const auto *EmplacyCall =
       Result.Nodes.getNodeAs<CXXMemberCallExpr>("emplacy_call");
-  const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctor");
+  auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctor");
+  auto *AggInitCall = Result.Nodes.getNodeAs<InitListExpr>("agg_init");
   const auto *MakeCall = Result.Nodes.getNodeAs<CallExpr>("make");
   const auto *TemporaryExpr =
       Result.Nodes.getNodeAs<MaterializeTemporaryExpr>("temporary_expr");
@@ -332,19 +376,42 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
   }();
 
   assert(Call && "No call matched");
-  assert((CtorCall || MakeCall) && "No push_back parameter matched");
+  assert((CtorCall || MakeCall || AggInitCall) &&
+         "No push_back parameter matched");
 
   if (IgnoreImplicitConstructors && CtorCall && CtorCall->getNumArgs() >= 1 &&
       CtorCall->getArg(0)->getSourceRange() == CtorCall->getSourceRange())
     return;
 
+  if (IgnoreImplicitConstructors && AggInitCall &&
+      AggInitCall->getNumInits() >= 1 &&
+      AggInitCall->getInit(0)->getSourceRange() ==
+          AggInitCall->getSourceRange())
+    return;
+
+  if (getLangOpts().LangStd >= LangStandard::lang_cxx20 && AggInitCall) {
+    for (const auto *Init : AggInitCall->inits()) {
+      if (const auto *InnerConstructorExpr = unwrapToConstructorExpr(Init)) {
+        // consume all args if it's an empty constructor call so that we can ->
+        // T{} -> emplace_back()
+        if (InnerConstructorExpr && InnerConstructorExpr->getNumArgs() == 0) {
+
+          CtorCall = InnerConstructorExpr;
+          AggInitCall = nullptr;
+          break;
+        }
+      }
+    }
+  }
+
   const auto FunctionNameSourceRange = CharSourceRange::getCharRange(
       Call->getExprLoc(), Call->getArg(0)->getExprLoc());
 
   auto Diag =
       EmplacyCall
           ? diag(TemporaryExpr ? TemporaryExpr->getBeginLoc()
                  : CtorCall    ? CtorCall->getBeginLoc()
+                 : AggInitCall ? AggInitCall->getBeginLoc()
                                : MakeCall->getBeginLoc(),
                  "unnecessary temporary object created while calling %0")
           : diag(Call->getExprLoc(), "use emplace%select{|_back|_front}0 "
@@ -376,9 +443,10 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
   }
 
   const SourceRange CallParensRange =
-      MakeCall ? SourceRange(MakeCall->getCallee()->getEndLoc(),
-                             MakeCall->getRParenLoc())
-               : CtorCall->getParenOrBraceRange();
+      MakeCall   ? SourceRange(MakeCall->getCallee()->getEndLoc(),
+                               MakeCall->getRParenLoc())
+      : CtorCall ? CtorCall->getParenOrBraceRange()
+                 : AggInitCall->getSourceRange();
 
   // Finish if there is no explicit constructor call.
   if (CallParensRange.getBegin().isInvalid())
@@ -387,6 +455,7 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
   // FIXME: Will there ever be a CtorCall, if there is no TemporaryExpr?
   const SourceLocation ExprBegin = TemporaryExpr ? TemporaryExpr->getExprLoc()
                                    : CtorCall    ? CtorCall->getExprLoc()
+                                   : AggInitCall ? AggInitCall->getExprLoc()
                                                  : MakeCall->getExprLoc();
 
   // Range for constructor name and opening brace.

clang-tools-extra/docs/ReleaseNotes.rst

@@ -182,6 +182,10 @@ Changes in existing checks
   ``constexpr`` and ``static``` values on member initialization and by detecting
   explicit casting of built-in types within member list initialization.
 
+- Improved :doc:`modernize-use-emplace
+  <clang-tidy/checks/modernize/use-emplace>` check by adding support for C++20
+  Argument forwarding inside ``struct`` type objects.
+
 - Improved :doc:`modernize-use-ranges
   <clang-tidy/checks/modernize/use-ranges>` check by updating suppress 
   warnings logic for ``nullptr`` in ``std::find``.

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

@@ -1,4 +1,12 @@
-// RUN: %check_clang_tidy %s modernize-use-emplace %t -- \
+// RUN: %check_clang_tidy %s -std=c++11 modernize-use-emplace %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN:             {modernize-use-emplace.ContainersWithPushBack: \
+// RUN:                '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector', \
+// RUN:              modernize-use-emplace.TupleTypes: \
+// RUN:                '::std::pair; std::tuple; ::test::Single', \
+// RUN:              modernize-use-emplace.TupleMakeFunctions: \
+// RUN:                '::std::make_pair; ::std::make_tuple; ::test::MakeSingle'}}"
+// RUN: %check_clang_tidy -std=c++20 %s modernize-use-emplace -check-suffixes=,CPP20 %t  -- \
 // RUN:   -config="{CheckOptions: \
 // RUN:             {modernize-use-emplace.ContainersWithPushBack: \
 // RUN:                '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector', \
@@ -1237,10 +1245,6 @@ void testBracedInitTemporaries() {
 
   // 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;
@@ -1288,8 +1292,6 @@ void testBracedInitTemporaries() {
   // 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;
@@ -1434,3 +1436,33 @@ void testWithPointerTypes() {
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: unnecessary temporary object created while calling emplace
   // CHECK-FIXES: sp->emplace();
 }
+
+namespace GH1225740 {
+
+void CXX20testBracedInitTemporaries(){
+
+  std::vector<NonTrivialNoCtor> v1;
+
+  v1.push_back(NonTrivialNoCtor{""});
+  // CHECK-MESSAGES-CPP20: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back
+  // CHECK-FIXES-CPP20: v1.emplace_back("");
+  v1.push_back({""});
+  // CHECK-MESSAGES-CPP20: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back
+  // CHECK-FIXES-CPP20: v1.emplace_back("");
+  v1.push_back(NonTrivialNoCtor{InnerType{""}});
+  // CHECK-MESSAGES-CPP20: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back
+  // CHECK-FIXES-CPP20: v1.emplace_back(InnerType{""});
+  v1.push_back({InnerType{""}});
+  // CHECK-MESSAGES-CPP20: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back
+  // CHECK-FIXES-CPP20: v1.emplace_back(InnerType{""});
+
+  std::vector<NonTrivialWithVector> v2;
+
+  v2.push_back(NonTrivialWithVector{std::vector<int>{0}});
+  // CHECK-MESSAGES-CPP20: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back
+  // CHECK-FIXES-CPP20: v2.emplace_back(std::vector<int>{0});
+  v2.push_back({std::vector<int>{0}});
+  // CHECK-MESSAGES-CPP20: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back
+  // CHECK-FIXES-CPP20: v2.emplace_back(std::vector<int>{0});
+  }
+}