[PATCH] D86544: [SyntaxTree] Add support for call expressions

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 accepted this revision.
gribozavr2 added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:333
+///   call-arguments:
+///     delimited_list(expression, ',', )
+/// Note: This construct is a simplification of the grammar rule for
----------------



================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:1070
+    Builder.markChildToken(
+        std::next(Builder.findToken(S->getCallee()->getEndLoc())),
+        syntax::NodeRole::OpenParen);
----------------
Could you add an assertion that it is indeed an open paren?

Or, rather, due to the decltype test, it has to be a check, and a FIXME that says that the check should become an assertion.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:1159
     case syntax::NodeKind::UnknownExpression:
-      return RecursiveASTVisitor::WalkUpFromCXXOperatorCallExpr(S);
+      return WalkUpFromExpr(S);
     default:
----------------
I don't understand this change to the unknown case, could you explain?


================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:2320
+
+TEST_P(SyntaxTreeTest, CallExpression_Callee_Operator) {
+  if (!GetParam().isCXX()) {
----------------



================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:2343
+
+TEST_P(SyntaxTreeTest, CallExpression_Callee_OperatorChaining) {
+  if (!GetParam().isCXX()) {
----------------



================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:2431-2436
+void f();
+void test() {
+  void (*pf)();
+  pf = &f;
+  [[(*pf)()]];
+}
----------------



================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:2437
+}
+)cpp",
+      {R"txt(
----------------
Please also add a test for `pf()` -- that's allowed, but I believe the AST is slightly different from `f()`.


================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:2734
+|   |   `-'args'
+|   `-'...'
+`-')' CloseParen
----------------
eduucaldas wrote:
> Here there is a divergence between the grammar and the ClangAST. 
> According to the [[ https://eel.is/c++draft/expr.post#nt:expression-list | grammar ]]  `...` would an optional element of `CallArguments`, but here `...` is part of an expression.
> 
> Perhaps I have used the wrong kind of `...`
> `...` would an optional element of CallArguments

I don't think so. Note that every element of initializer-list can have a `...` in it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86544/new/

https://reviews.llvm.org/D86544