[PATCH] D82954: Fix crash on overloaded postfix unary operators due to invalid SourceLocation

[Dmitri Gribenko via Phabricator via cfe-commits]

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