Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| clang/include/clang/ASTMatchers/ASTMatchers.h | ||
|---|---|---|
| 2867 | 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? | |
| clang/include/clang/ASTMatchers/ASTMatchers.h | ||
|---|---|---|
| 2867 | 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. | |
Adding some reviewers to see if we can bikeshed a somewhat better name than callOrConstruct, or to see if my concerns are unfounded.
| clang/include/clang/ASTMatchers/ASTMatchers.h | ||
|---|---|---|
| 2867 |
Yes, it's an MS extension.
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.
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? | |
| clang/include/clang/ASTMatchers/ASTMatchers.h | ||
|---|---|---|
| 2867 |
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. | |
Pinging the reviewers to help with the naming questions.
| clang/include/clang/ASTMatchers/ASTMatchers.h | ||
|---|---|---|
| 2867 |
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). | |
| clang/include/clang/ASTMatchers/ASTMatchers.h | ||
|---|---|---|
| 2867 | 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(). | |
| clang/include/clang/ASTMatchers/ASTMatchers.h | ||
|---|---|---|
| 2867 | I like invocation(), let's go with that one. | |
LGTM aside from the list order nit. Thanks!
| clang/lib/ASTMatchers/Dynamic/Registry.cpp | ||
|---|---|---|
| 155 | Please keep this sorted alphabetically. | |
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?