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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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) {
----------------
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.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:163
+inline bool isArrayOperator(const CXXDeleteExpr &Node) {
+  return Node.isArrayFormAsWritten();
+}
----------------
njames93 wrote:
> aaron.ballman wrote:
> > Do we need/want this interface to consider whether the user is matching things spelled in source or not?
> From what I can see the only time isArrayFrom and isArrayFromAsWritten differentiate is if normal `delete` operator is used on an arrayType. However that behaviour isn't standard afaik and was only added to be consistent with gcc and edg. If that case comes up we emit a fixit to add the `[]`.
> Maybe there is merit to check traversal kind before assuming tho.
> 
I think it's defensible either way, so I leave it to whatever you and @steveire think is appropriate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97681