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

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas added inline comments.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:1070
+    Builder.markChildToken(
+        std::next(Builder.findToken(S->getCallee()->getEndLoc())),
+        syntax::NodeRole::OpenParen);
----------------
gribozavr2 wrote:
> 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.
Thanks! It is indeed what I should've done :) 


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

In this change we added an overload for `WalkUpFromCallExpr`, so we started intercepting the `WalkUp` at `CallExpr` instead of `Expr`, this works around that.



================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:2734
+|   |   `-'args'
+|   `-'...'
+`-')' CloseParen
----------------
gribozavr2 wrote:
> 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.
True! I hadn't seen it correctly!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86544