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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:23
 
+AST_MATCHER(CXXStdInitializerListExpr, cxxStdInitializerListExpr) {
+  return true;
----------------
kuhar wrote:
> Prazek wrote:
> > kuhar wrote:
> > > alexfh wrote:
> > > > 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.
> > > 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?  
> > Why adding new matcher to 4.0.1 release is bad? Will it break anything?
> 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
> 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.)


================
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:24
+AST_MATCHER(CXXStdInitializerListExpr, cxxStdInitializerListExpr) {
+  return true;
+}
----------------
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.


https://reviews.llvm.org/D32767