aaron.ballman added reviewers: klimek, sammccall, dblaikie, echristo.
aaron.ballman added a comment.
Adding some reviewers to see if we can bikeshed a somewhat better name than
`callOrConstruct`, or to see if my concerns are unfounded.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2867
+extern const internal::MapAnyOfMatcher<CallExpr, CXXConstructExpr>
+ callOrConstruct;
+
----------------
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?
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
