steveire added inline comments.
================ Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2867 +extern const internal::MapAnyOfMatcher<CallExpr, CXXConstructExpr> + callOrConstruct; + ---------------- aaron.ballman wrote: > steveire wrote: > > aaron.ballman wrote: > > > steveire wrote: > > > > aaron.ballman wrote: > > > > > I'm not super keen on this name. It's certainly descriptive, but I do > > > > > wonder if it's a bit too specific and should perhaps be something > > > > > more like `callableExpr()`, `callLikeExpr()`, or something more > > > > > generic. For instance, I could imagine wanting this to match on > > > > > something like: > > > > > ``` > > > > > struct S { > > > > > void setter(int val) {} > > > > > __declspec(property(put = setter)) int x; > > > > > }; > > > > > > > > > > int main() { > > > > > S s; > > > > > s.x = 12; // Match here > > > > > // Because the above code actually does this: > > > > > // s.setter(12); > > > > > } > > > > > ``` > > > > > because this also has an expression that isn't really a call (as far > > > > > as our AST is concerned) but is a call as far as program semantics > > > > > are concerned. I'm not suggesting to make the matcher support that > > > > > right now (unless you felt like doing it), but thinking about the > > > > > future and avoiding a name that may paint us into a corner. > > > > > > > > > > WDYT about using a more generic name? > > > > I haven't seen code like that before (ms extension?) > > > > https://godbolt.org/z/anvd43 but I think that should be matched by > > > > `binaryOperator` instead. That already matches based on what the code > > > > looks like, rather than what it is in the AST. > > > > > > > > This `callOrConstruct` is really for using `hasArgument` and related > > > > submatchers with nodes which support it. As such I think the name is > > > > fine. I don't like `callableExpr` or `callLikeExpr` because they don't > > > > bring to mind the possibility that construction is also supported. > > > > I haven't seen code like that before (ms extension?) > > > > > > Yes, it's an MS extension. > > > > > > > That already matches based on what the code looks like, rather than > > > > what it is in the AST. > > > > > > Yes, but these are AST matchers, so it's reasonable to match on what's in > > > the AST (as well as what the code looks like, of course). I'm not arguing > > > it needs to be supported so much as pointing out that there are other AST > > > nodes this notionally applies to where the name is a bit too specific. > > > > > > > This callOrConstruct is really for using hasArgument and related > > > > submatchers with nodes which support it. As such I think the name is > > > > fine. I don't like callableExpr or callLikeExpr because they don't > > > > bring to mind the possibility that construction is also supported. > > > > > > I'm pretty sure we've extended what `hasArgument` can be applied to in > > > the past (but I've not verified), so the part that worries me is > > > specifically naming the nodes as part of the identifier. This effectively > > > means that if we ever find another AST node for `hasArgument`, we either > > > need a different API like `callConstructOrWhatever` or we're stuck with a > > > poor name. > > > > > > Another (smaller) concern with the name is that `callOrConstruct` can > > > describe declarations as well as expressions, to some degree as you can > > > declare calls and constructors. It's a smaller concern because those at > > > least share a common base class. `callOrConstructExpr` would clarify this > > > easily enough. > > > > > > I see you added `ObjCMessageExpr` as well, thank you for that! It's a > > > perhaps better example of why this name feels awkward to me. In ObjC, you > > > don't call an `ObjCMessageExpr`, you "send" it to the given object or > > > class. That suggests to me that `callableExpr` or `callLikeExpr` is also > > > not a great name either. > > > > > > Perhaps `executableExpr` because you're executing some code? > > > Perhaps `executableExpr` because you're executing some code? > > > > The thing that this really does is make it possible to use `hasArgument` > > and related matchers with the nodes that that matcher supports. So, > > something with `argument` in the name probably makes sense. Like > > `argumentExpr`. > > > > The thing that this really does is make it possible to use hasArgument and > > related matchers with the nodes that that matcher supports. So, something > > with argument in the name probably makes sense. Like argumentExpr. > > A name like `argumentExpr()` would make me think we're trying to match the > `42` in an expression like `foo(42)` (e.g., it makes me think we're going to > match on expressions that are arguments to a call). Actually I think that's confusing. Other matchers with `Expr` suffix are for matching subclasses of `clang::Expr`. This name would break that mould. So, I think it should be `invocation()`, which follows well from `binaryOperation()`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94865/new/ https://reviews.llvm.org/D94865 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits