[PATCH] D97681: [ASTMatchers] Add matchers for `CXXDeleteExpr`

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:7621
+///   matches the expression 'delete Ptr'.
+AST_MATCHER_P_OVERLOAD(CXXDeleteExpr, deletes, internal::Matcher<Expr>,
+                       InnerMatcher, 1) {
----------------
aaron.ballman wrote:
> njames93 wrote:
> > aaron.ballman wrote:
> > > Why add this instead of continuing to use `has()`? (I worry a bit that we'll want to add a verb for different subexpression matchers and it won't scale well or we won't add verbs for different subexpressions and this'll introduce a new inconsistency.)
> > This change is about lowering the barrier to entry. While `has` can be very powerful, its interface isn't newcomer friendly. 
> > You can put any matcher in, that may not make sense or ever be capable of actually matching but it will happily compile without emitting any warning. using `deletes` forces some type checking on the argument you provide.
> > The explicit names are also much better for documentation and API discovery.
> > When running in clang query with completions this is what appears at the top of the list
> > ```
> > clang-query> m cxxDeleteExpr(
> > Matcher<CXXDeleteExpr> deletes(Matcher<Expr>)
> > Matcher<CXXDeleteExpr> isArray()
> > ```
> > A similar case is made for the matchers documentation.
> Thanks! I agree with your assessment about this being a more user-friendly interface. My concerns are more mundane though. IIRC, each time we add a new matcher, we slow down compilation for the entire Clang project by a nontrivial amount because of the large amount of template instantiations involved. Because of that, we usually only add matchers when we find there's a need for them (as opposed to generating matchers for everything possible). So I worry that we'll slow down compile times and increase build sizes for a relatively uncommon AST matcher that isn't strictly necessary, and that it'll be a slippery slope to do more of this. That said, I've not measured the slowdown and perhaps things have improved here.
> 
> @klimek may have more context or thoughts on this.
That is a good point, maybe though if we felt so inclined. We could actually split up the Matcher definitions from their declarations. It may not work as nicely with PolymorphicMatchers but given around 3/4 of the matchers aren't, It could have a positive impact on compile time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97681/new/

https://reviews.llvm.org/D97681