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

 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;"
     "deque::emplace; deque::emplace_front; deque::emplace_back;"
     "forward_list::emplace_after; forward_list::emplace_front;"
     "list::emplace; list::emplace_back; list::emplace_front;"
     "set::emplace; set::emplace_hint;"
     "map::emplace; map::emplace_hint;"
     "multiset::emplace; multiset::emplace_hint;"
     "multimap::emplace; multimap::emplace_hint;"
     "unordered_set::emplace; unordered_set::emplace_hint;"
     "unordered_map::emplace; unordered_map::emplace_hint;"
     "unordered_multiset::emplace; unordered_multiset::emplace_hint;"
     "unordered_multimap::emplace; unordered_multimap::emplace_hint;"
     "stack::emplace; queue::emplace; priority_queue::emplace";
 } // namespace
 
: ClangTidyCheck(Name, Context), IgnoreImplicitConstructors(Options.get(
           Options.get("EmplacyFunctions", DefaultEmplacyFunctions))) {}
 
 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 PushBackTypes = hasCanonicalType(
+      hasDeclaration(cxxRecordDecl(hasAnyName(ContainersWithPushBack))));
   auto CallPushBack = cxxMemberCallExpr(
       hasDeclaration(functionDecl(hasName("push_back"))),
-      on(hasType(hasCanonicalType(
-          hasDeclaration(cxxRecordDecl(hasAnyName(ContainersWithPushBack)))))));
+      on(anyOf(hasType(PushBackTypes),
+               hasType(pointerType(pointee(PushBackTypes))))));
 
-  auto CallPush =
-      cxxMemberCallExpr(hasDeclaration(functionDecl(hasName("push"))),
-                        on(hasType(hasCanonicalType(hasDeclaration(
-                            cxxRecordDecl(hasAnyName(ContainersWithPush)))))));
+  auto PushTypes = hasCanonicalType(
+      hasDeclaration(cxxRecordDecl(hasAnyName(ContainersWithPush))));
+  auto CallPush = cxxMemberCallExpr(
+      hasDeclaration(functionDecl(hasName("push"))),
+      on(anyOf(hasType(PushTypes), hasType(pointerType(pointee(PushTypes))))));
 
+  auto PushFrontTypes = hasCanonicalType(
+      hasDeclaration(cxxRecordDecl(hasAnyName(ContainersWithPushFront))));
   auto CallPushFront = cxxMemberCallExpr(
       hasDeclaration(functionDecl(hasName("push_front"))),
-      on(hasType(hasCanonicalType(hasDeclaration(
-          cxxRecordDecl(hasAnyName(ContainersWithPushFront)))))));
+      on(anyOf(hasType(PushFrontTypes),
+               hasType(pointerType(pointee(PushFrontTypes))))));
 
-  auto CallEmplacy = cxxMemberCallExpr(
-      hasDeclaration(
-          functionDecl(hasAnyNameIgnoringTemplates(EmplacyFunctions))),
-      on(hasType(hasCanonicalType(hasDeclaration(has(typedefNameDecl(
-          hasName("value_type"), hasType(type(hasUnqualifiedDesugaredType(
-                                     recordType().bind("value_type")))))))))));
+  auto EmplacyTypes = hasCanonicalType(hasDeclaration(has(typedefNameDecl(
+                                 recordType().bind("value_type"))))))));
+  auto CallEmplacy =
+      cxxMemberCallExpr(hasDeclaration(functionDecl(
+                            hasAnyNameIgnoringTemplates(EmplacyFunctions))),
+                        on(anyOf(hasType(EmplacyTypes),
+                                 hasType(pointerType(pointee(EmplacyTypes))))));
 
   // 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::vector<Something> v;
+  std::vector<Something>* vp = &v;
+
+  vp->push_back(Something(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace_back instead of push_back [modernize-use-emplace]
+  // CHECK-FIXES: vp->emplace_back(1, 2);
+
+  vp->push_back(Something{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace_back instead of push_back [modernize-use-emplace]
+  // CHECK-FIXES: vp->emplace_back(1, 2);
+
+  vp->push_back(Something());
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace_back instead of push_back [modernize-use-emplace]
+  // CHECK-FIXES: vp->emplace_back();
+
+  vp->push_back(Something{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use emplace_back instead of push_back [modernize-use-emplace]
+  // CHECK-FIXES: vp->emplace_back();
+
+  vp->emplace_back(Something(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: vp->emplace_back(1, 2);
+
+  vp->emplace_back(Something{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: vp->emplace_back(1, 2);
+
+  vp->emplace_back(Something());
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: vp->emplace_back();
+
+  vp->emplace_back(Something{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: unnecessary temporary object created while calling emplace_back
+  // CHECK-FIXES: vp->emplace_back();
+}