eduucaldas added a comment.
================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:741
bool WalkUpFromIntegerLiteral(IntegerLiteral *S) {
+ if (S->getLocation().isInvalid())
+ return true;
----------------
gribozavr2 wrote:
> 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.
>>! In D82954#2125300, @eduucaldas wrote:
> [[ https://godbolt.org/z/CWVEJ2 | Code that reproduces the crash ]]
> Notice that when using a postfix unary operator ++ one argument is introduced
> to differ it from its prefix counterpart, but that argument is not used. This
> phantom argument shows up in the ClangAST in the form of an `IntegerLiteral`
> with `invalid sloc`. This invalid sloc in a valid AST node was causing the
> SyntaxTree generation to crash.
> We can address this problems at two different levels:
> 1. At the `TraverseCXXOperatorCallExpr`, by overriding it and replacing the
> children iteration to not iterate over this phantom argument.
> 2. At the `WalkUpFromIntegerLiteral`, by skipping the visitation of the
> phantom node.
> We preferred the latter.
> 1. Cons: We would have to duplicate the implementation of RecursiveASTVisitor
> in BuildTree, to handle this subtle change in traversal. That would add code
> complexity.
> 2. Cons: We are handling a problem of `CXXOperatorCallExpr` in
> `IntegerLiteral`.
>
> We chose the latter as, anyways, if an AST node has an invalid sloc, it was
> either a problem with parsing or the node is supposed to be ignored
I've explained my reasoning in my first comment for this patch. But as it was a
long time ago, I guess it got lost, even by me.
I'll sketch how the Traverse solution would look like, to be able to give more
concrete arguments.
================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:2376
+ | | | `-x
+ | | `-IdExpression
+ | | `-UnqualifiedId
----------------
gribozavr2 wrote:
> 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)?
This comes back to a discussion we had a month ago about operators ( `+`, `!`,
etc)
**Question**: Should we represent operators (built-in or overloaded) in the
syntax tree uniformly? If so in which way?
**Context**: The ClangAST stores built-in operators as mere tokens, whereas
overloaded operators are represented as a `DeclRefExpr`. That makes a lot of
sense for the ClangAST, as we might refer back to the declaration of the
overloaded operator, but the declaration of built-in operator doesn't exist.
**Conclusion**: Have the same representation for both types of operators. I
have implemented the "unboxed" representation of overloaded operators, i.e.
just storing their token in the syntax tree. I have not committed that, but I
can do it just after this patch.
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
