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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
