Fix handling of -> calls for modernize-use-emplace

[BigPeet]

Summary

Fix handling of -> calls for modernize-use-emplace

Fixes #55869

[nicovank]

This is becoming repetitive, but I guess that's the nature of those things. Maybe something like this would help clean it up, not sure if any better for right now.

template <typename T> // Lambda templates would have been nice.
auto onTypeOrPointerType(const T &Type) { // onUnderlyingType ?
  return on(anyOf(hasType(Type), hasType(pointerType(pointee(Type)))));
}

[BigPeet]

• reducing code repetition + further tests

[BigPeet]

• additional deduplication of code

[BigPeet]

Refactored the code a little bit to get rid of some repetitions. Should effectively match the same code as the accepted revision.

[BigPeet]

• fix passing vector by value instead of by ref-to-const

[nicovank]

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

100

const std::vector<StringRef> &ContainerNames

[nicovank]

Nevermind, this was fixed as I was looking at it. LGTM.

[njames93]

LGTM

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

93

Generally we use prefer llvm::ArrayRef over const std::vector &

[BigPeet]

Generally we use prefer llvm::ArrayRef over const std::vector &

I will keep this in mind. Or should this be fixed in this revision?

Anyway, if this gets merged, my username/mail are

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

Thanks for the fast review response!

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

93

Generally we use prefer llvm::ArrayRef over const std::vector &

[BigPeet]

Ping.

[carlosgalvezp]

Pushed, thanks for the contribution! I took the liberty of fixing the std::vector thing before pushing :)

[BigPeet]

Thank you very much.

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

AST_MATCHER(CXXMemberCallExpr, hasSameNumArgsAsDeclNumParams) {
 
   return Node.getNumArgs() == Node.getMethodDecl()->getNumParams();
 }
 
 AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
   return Node.hasExplicitTemplateArgs();
 }
 
+// Helper Matcher which applies the given QualType Matcher either directly or by
+// resolving a pointer type to its pointee. Used to match v.push_back() as well
+// as p->push_back().
+auto hasTypeOrPointeeType(
+    const ast_matchers::internal::Matcher<QualType> &TypeMatcher) {
+  return anyOf(hasType(TypeMatcher),
+               hasType(pointerType(pointee(TypeMatcher))));
+}
+
+// Matches if the node has canonical type matching any of the given names.
+auto hasWantedType(const std::vector<StringRef> &TypeNames) {
+  return hasCanonicalType(hasDeclaration(cxxRecordDecl(hasAnyName(TypeNames))));
+}
+
 const auto DefaultContainersWithPushBack =
     "::std::vector; ::std::list; ::std::deque";
 const auto DefaultContainersWithPush =
     "::std::stack; ::std::queue; ::std::priority_queue";
 const auto DefaultContainersWithPushFront =
     "::std::forward_list; ::std::list; ::std::deque";
 const auto DefaultSmartPointers =
     "::std::shared_ptr; ::std::unique_ptr; ::std::auto_ptr; ::std::weak_ptr";
 const auto DefaultTupleTypes = "::std::pair; ::std::tuple";
 const auto DefaultTupleMakeFunctions = "::std::make_pair; ::std::make_tuple";
 const auto DefaultEmplacyFunctions =
     "vector::emplace_back; vector::emplace;"

 void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
   // FIXME: Bunch of functionality that could be easily added:
   // + add handling of `insert` for stl associative container, but be careful
   // because this requires special treatment (it could cause performance
   // regression)
   // + match for emplace calls that should be replaced with insertion
   auto CallPushBack = cxxMemberCallExpr(
       hasDeclaration(functionDecl(hasName("push_back"))),
-      on(hasType(hasCanonicalType(
-          hasDeclaration(cxxRecordDecl(hasAnyName(ContainersWithPushBack)))))));
+      on(hasTypeOrPointeeType(hasWantedType(ContainersWithPushBack))));
 
-  auto CallPush =
-      cxxMemberCallExpr(hasDeclaration(functionDecl(hasName("push"))),
-                        on(hasType(hasCanonicalType(hasDeclaration(
-                            cxxRecordDecl(hasAnyName(ContainersWithPush)))))));
+  auto CallPush = cxxMemberCallExpr(
+      hasDeclaration(functionDecl(hasName("push"))),
+      on(hasTypeOrPointeeType(hasWantedType(ContainersWithPush))));
 
   auto CallPushFront = cxxMemberCallExpr(
       hasDeclaration(functionDecl(hasName("push_front"))),
-      on(hasType(hasCanonicalType(hasDeclaration(
-          cxxRecordDecl(hasAnyName(ContainersWithPushFront)))))));
+      on(hasTypeOrPointeeType(hasWantedType(ContainersWithPushFront))));
 
   auto CallEmplacy = cxxMemberCallExpr(
       hasDeclaration(
           functionDecl(hasAnyNameIgnoringTemplates(EmplacyFunctions))),
-      on(hasType(hasCanonicalType(hasDeclaration(has(typedefNameDecl(
-          hasName("value_type"), hasType(type(hasUnqualifiedDesugaredType(
-                                     recordType().bind("value_type")))))))))));
+      on(hasTypeOrPointeeType(hasCanonicalType(hasDeclaration(
+          has(typedefNameDecl(hasName("value_type"),
+                              hasType(type(hasUnqualifiedDesugaredType(
 
   // We can't replace push_backs of smart pointer because
   // if emplacement fails (f.e. bad_alloc in vector) we will have leak of
   // passed pointer because smart pointer won't be constructed
   // (and destructed) as in push_back case.
   auto IsCtorOfSmartPtr =
       hasDeclaration(cxxConstructorDecl(ofClass(hasAnyName(SmartPointers))));
 

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

void testBracedInitTemporaries() {
 
   // 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({{}});
 }
+
+void testWithPointerTypes() {
+  std::list<Something> l;
+  std::list<Something>* lp = &l;
+  std::stack<Something> s;
+  std::stack<Something>* sp;
+
+  lp->push_back(Something(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace_back instead of push_back [modernize-use-emplace]
+  // CHECK-FIXES: lp->emplace_back(1, 2);
+  lp->push_front(Something(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace_front instead of push_front [modernize-use-emplace]
+  // CHECK-FIXES: lp->emplace_front(1, 2);
+  sp->push(Something(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace instead of push [modernize-use-emplace]
+  // CHECK-FIXES: sp->emplace(1, 2);
+
+  lp->push_back(Something{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace_back instead of push_back [modernize-use-emplace]
+  // CHECK-FIXES: lp->emplace_back(1, 2);
+  lp->push_front(Something{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace_front instead of push_front [modernize-use-emplace]
+  // CHECK-FIXES: lp->emplace_front(1, 2);
+  sp->push(Something{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace instead of push [modernize-use-emplace]
+  // CHECK-FIXES: sp->emplace(1, 2);
+
+  lp->push_back(Something());
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace_back instead of push_back [modernize-use-emplace]
+  // CHECK-FIXES: lp->emplace_back();
+  lp->push_front(Something());
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace_front instead of push_front [modernize-use-emplace]
+  // CHECK-FIXES: lp->emplace_front();
+  sp->push(Something());
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace instead of push [modernize-use-emplace]
+  // CHECK-FIXES: sp->emplace();
+
+  lp->push_back(Something{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace_back instead of push_back [modernize-use-emplace]
+  // CHECK-FIXES: lp->emplace_back();
+  lp->push_front(Something{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace_front instead of push_front [modernize-use-emplace]
+  // CHECK-FIXES: lp->emplace_front();
+  sp->push(Something{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace instead of push [modernize-use-emplace]
+  // CHECK-FIXES: sp->emplace();
+
+  lp->emplace_back(Something(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: lp->emplace_back(1, 2);
+  lp->emplace_front(Something(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: unnecessary temporary object created while calling emplace_front
+  // CHECK-FIXES: lp->emplace_front(1, 2);
+  sp->emplace(Something(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: unnecessary temporary object created while calling emplace
+  // CHECK-FIXES: sp->emplace(1, 2);
+
+  lp->emplace_back(Something{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: lp->emplace_back(1, 2);
+  lp->emplace_front(Something{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: unnecessary temporary object created while calling emplace_front
+  // CHECK-FIXES: lp->emplace_front(1, 2);
+  sp->emplace(Something{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: unnecessary temporary object created while calling emplace
+  // CHECK-FIXES: sp->emplace(1, 2);
+
+  lp->emplace_back(Something());
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: lp->emplace_back();
+  lp->emplace_front(Something());
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: unnecessary temporary object created while calling emplace_front
+  // CHECK-FIXES: lp->emplace_front();
+  sp->emplace(Something());
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: unnecessary temporary object created while calling emplace
+  // CHECK-FIXES: sp->emplace();
+
+  lp->emplace_back(Something{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: lp->emplace_back();
+  lp->emplace_front(Something{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: unnecessary temporary object created while calling emplace_front
+  // CHECK-FIXES: lp->emplace_front();
+  sp->emplace(Something{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: unnecessary temporary object created while calling emplace
+  // CHECK-FIXES: sp->emplace();
+}