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