gribozavr2 added inline comments.
================ Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:118 +syntax::NodeKind getOperatorNodeKind(const CXXOperatorCallExpr &E) { + switch (E.getOperator()) { ---------------- eduucaldas wrote: > # Where to put this logic? > The pro of having this function inside `BuildTree.cpp` is that it is closer > to it's *only* usage. And also for now `BuildTree.cpp` agglomerates > everything that takes a semantic AST as an input, so it would be coherent. > > Another option is to put it in `Nodes`, as it might serve as documentation > for `*OperatorExpression`s. > For example, it would document that the semantic node `CXXOperatorCallExpr` > can also generate the syntax node `BinaryOperatorExpression`, which was > previously only generated by a semantic `BinaryOperator` > > Another option is to add put this function as a lambda inside > `WalkUpFromCXXOperatorCallExpr` as probably it will only be used there. Placing this helper here makes sense to me. However, please mark it static. ================ Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:178 + return syntax::NodeKind::BinaryOperatorExpression; + // Not supported by SyntaxTree + case OO_New: ---------------- "Not supported by syntax tree *yet*" ================ Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:741 bool WalkUpFromIntegerLiteral(IntegerLiteral *S) { + if (S->getLocation().isInvalid()) + return true; ---------------- WDYT about overriding `TraverseCXXOperatorCallExpr`, so that we don't even visit the synthetic argument of the postfix unary `++`? I would prefer to not introduce blanket "if invalid then ignore" checks in the code. ================ Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:852 + default: + llvm_unreachable("getKind does not implement that"); } ---------------- "getOperatorNodeKind() does not return this value" ================ Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:2333 +struct X { + X operator++(int); +}; ---------------- Could you also add tests for a prefix unary operator? ================ Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:2376 + | | | `-x + | | `-IdExpression + | | `-UnqualifiedId ---------------- I'm not sure about this part where `++` is wrapped in IdExpression -- shouldn't the syntax tree look identical to a builtin postfix `++` (see another test in this file)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82954/new/ https://reviews.llvm.org/D82954 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits