[PATCH] D82954: Fix crash on overloaded postfix unary operators due to invalid SourceLocation
Dmitri Gribenko via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 7 04:13:59 PDT 2020
gribozavr2 accepted this revision.
gribozavr2 added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:149
+ case OO_PipePipe:
+ // Less common binary operators
+ case OO_ArrowStar:
----------------
Please drop this comment, I don't think it helps understand the code.
================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:2192
R"cpp(
+class osstream {};
struct X {
----------------
I don't think we need a separate class to show the left shift operator. The declaration below can be:
```
friend X operator<<(X&, const X&);
```
================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:2199
+ X operator,(X&);
+ // TODO: Test for `->*`. Fix crash before
};
----------------
"TODO: Fix the crash on `->*` and add a test for it."
================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:2376
+ | | | `-x
+ | | `-IdExpression
+ | | `-UnqualifiedId
----------------
eduucaldas wrote:
> 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.
SGTM. Please just leave a FIXME or TODO in this patch that shows that certain parts of tests are not final.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82954/new/
https://reviews.llvm.org/D82954
More information about the cfe-commits
mailing list