aaron.ballman added inline comments.
================
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:
> > > > 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()`.
I like `invocation()`, let's go with that one.
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
