[PATCH] D94865: [ASTMatchers] Add callOrConstruct matcher

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Pinging the reviewers to help with the naming questions.



================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2867
+extern const internal::MapAnyOfMatcher<CallExpr, CXXConstructExpr>
+    callOrConstruct;
+
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94865