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