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