[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:
> 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.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:163
+inline bool isArrayOperator(const CXXDeleteExpr &Node) {
+  return Node.isArrayFormAsWritten();
+}
----------------
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.



================
Comment at: clang/lib/ASTMatchers/Dynamic/Registry.cpp:269
   REGISTER_MATCHER(hasAnyUsingShadowDecl);
-  REGISTER_MATCHER(hasArgument);
   REGISTER_MATCHER(hasArgumentOfType);
----------------
aaron.ballman wrote:
> Why removing this?
Accident. Will re add.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97681