[clang-tidy] Fix PR32896: detect initializer lists in modernize-use-empalce

[kuhar]

Summary

This patch fixes PR32896.

The problem was that modernize-use-emplace incorrectly removed changed push_back into emplace_back, removing explicit constructor call with initializer list parameter, resulting in compiler error after applying fixits.
modernize-use-emplace used to check if matched constructor had InitListExpr, but didn't check against CXXStdInitializerListExpr.

Eg.

std::vector<std::vector<int>> v;
  v.push_back(std::vector<int>({1})); // --> v.emplace_back({1});

[alexfh]

Thank you! One comment inline.

clang-tidy/modernize/UseEmplaceCheck.cpp

23 ↗

(On Diff #97516)

This should be a node matcher rather than a narrowing matcher, and it should be placed to ASTMatchers.h, if there is no such matcher already.

[kuhar]

clang-tidy/modernize/UseEmplaceCheck.cpp

23 ↗

(On Diff #97516)

I wanted to backport this fix to 4.0.1 release after applying it in trunk.
I thought that adding a new ASTMatcher would introduce an API change in clang, and I'm not sure if it's much welcome.

Maybe it would be better to introduce a new matcher in ASTMatchers.h in trunk and make it an internal narrowing matcher in the backported patch? What would be best?

[Prazek]

clang-tidy/modernize/UseEmplaceCheck.cpp

23 ↗

(On Diff #97516)

Why adding new matcher to 4.0.1 release is bad? Will it break anything?

[kuhar]

clang-tidy/modernize/UseEmplaceCheck.cpp

23 ↗

(On Diff #97516)

The example that I originally came up with was updating clang-tidy to 4.0.1 while keeping clang libs 4.0.0, but since we just include all the matchers I think it should work fine.

Here's a patch with the new matcher: https://reviews.llvm.org/D32810

[aaron.ballman]

clang-tidy/modernize/UseEmplaceCheck.cpp

23 ↗

(On Diff #97516)

Why adding new matcher to 4.0.1 release is bad? Will it break anything?

Because it doesn't meet the criteria for doing a point release, generally speaking.

I think a better approach is to leave this matcher as part of this patch, get the patch submitted, then Tom can cherrypick the commit for 4.0.1. You can then submit a second patch to hoist this matcher out to ASTMatchers.h and use it within this check. (Basically do it in two steps.)

24 ↗

(On Diff #97516)

This looks odd to me. Can you do:

const internal::VariadicDynCastAllOfMatcher<Expr, CXXStdInitializerListExpr> cxxStdInitializerListExpr;

I'm not certain whether this works or not, but we don't have matchers that just return true.

[kuhar]

I switched from a narrowing matcher to a "normal" node matcher.

[aaron.ballman]

LGTM!

[alexfh]

LG

[Prazek]

LGTM

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

 namespace tidy {
 namespace modernize {
 
 namespace {
 AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
   return Node.hasExplicitTemplateArgs();
 }
 
+namespace impl {
+// FIXME: This matcher should be replaced by a matcher from ASTMatcher.h
+const ast_matchers::internal::VariadicDynCastAllOfMatcher<Stmt,
+    CXXStdInitializerListExpr> cxxStdInitializerListExpr;
+} // namespace impl
+
 const auto DefaultContainersWithPushBack =
     "::std::vector; ::std::list; ::std::deque";
 const auto DefaultSmartPointers =
     "::std::shared_ptr; ::std::unique_ptr; ::std::auto_ptr; ::std::weak_ptr";
 } // namespace
 
 UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
   auto NewExprAsArgument = hasAnyArgument(ignoringImplicit(cxxNewExpr()));
   // We would call another constructor.
   auto ConstructingDerived =
       hasParent(implicitCastExpr(hasCastKind(CastKind::CK_DerivedToBase)));
 
   // emplace_back can't access private constructor.
   auto IsPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
 
-  auto HasInitList = has(ignoringImplicit(initListExpr()));
+  auto HasInitList = anyOf(has(ignoringImplicit(initListExpr())),
+                           has(impl::cxxStdInitializerListExpr()));
+  // FIXME: Replace internal C++ initializer list matcher with one from
+  // ASTMatchers.h
+
   // 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, IsPrivateCtor)))
           .bind("ctor");

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

 // RUN: %check_clang_tidy %s modernize-use-emplace %t -- \
 // RUN:   -config="{CheckOptions: \
 // RUN:             [{key: modernize-use-emplace.ContainersWithPushBack, \
 // RUN:               value: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector'}]}" -- -std=c++11
 
 namespace std {
+template <typename>
+class initializer_list
+{
+public:
+  initializer_list() noexcept {}
+};
+
 template <typename T>
 class vector {
 public:
+  vector() = default;
+  vector(initializer_list<T>) {}
+
   void push_back(const T &) {}
   void push_back(T &&) {}
 
   template <typename... Args>
   void emplace_back(Args &&... args){};
   ~vector();
 };
 template <typename T>

 
 void testWithDtor() {
   std::vector<WithDtor> v;
 
   v.push_back(WithDtor(42));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: v.emplace_back(42);
 }
+
+void testInitializerList() {
+  std::vector<std::vector<int>> v;
+  v.push_back(std::vector<int>({1}));
+  // Test against the bug reported in PR32896.
+
+  v.push_back({{2}});
+
+  using PairIntVector = std::pair<int, std::vector<int>>;
+  std::vector<PairIntVector> x;
+  x.push_back(PairIntVector(3, {4}));
+  x.push_back({5, {6}});
+}