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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
