[PATCH] D61637: [Syntax] Introduce syntax trees

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:35
+/// A root node for a translation unit. Parent is always null.
+class TranslationUnitDeclaration final : public Tree {
+public:
----------------
sammccall wrote:
> I don't think TU is actually a declaration. Is there a reason to consider it one from a syntax POV?
It's a declaration in a sense that it has a corresponding instance of `clang::Decl` that it "introduces", i.e. the `clang::TranslationUnitDecl`.

But you are right, `TranslationUnit` is a better name: this aligns with the C++ grammar from the standard (`translation-unit`), it does lack similarity with other declarations from the standard (and from clang).


Renamed to `TranslationUnit`


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:41
+  }
+  syntax::Leaf *eof();
+
----------------
sammccall wrote:
> I have a slight feeling the EOF token is going to be annoying, e.g. can't just splice stuff in at the end of the list. But not sure if it'll be a big deal, and whether the alternatives are better.
That's a good point, I actually don't see how `eof` token in a tree would be useful (it's probably fine to have in the `TokenBuffer`, though, allows). Moreover, it could cause confusion and bugs when working with multiple translation units (I imagine moving nodes between two different TUs and ending up with multiple `eof`s somewhere) 

I've removed the corresponding accessor from the `TranslationUnit` node and added a FIXME to remove it from the tree altogether.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:43
+
+  struct Roles {
+    static constexpr NodeRole eof = 1;
----------------
sammccall wrote:
> we discussed offline - with 256 values, is it possible to come up with a single role enum that would cover all node types?
> 
> Advantage would be that certain logic could be generic (e.g. `Recovery` could be a role for leaves under any Tree, `LParen`/`RParen`/`MainKeyword` could apply to if, while, switch...)
Will need to do some estimations to answer this properly, but my gut feeling is that 256 could end up being too limiting in the long run (I would expect each node to have at least one child, so without deduplication we can at least as many roles as we have kinds).

Could imagine a two-level numbering scheme, though:
- some generic roles like `lparen`, `rparen`, etc, take first `N` roles.
- higher numbers are for node-specific roles (e.g. LHS or RHS of a `BinaryExpr`).

But at that point, we probably don't have the benefits of a single enum.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61637/new/

https://reviews.llvm.org/D61637