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
