[PATCH] D61637: [Syntax] Introduce syntax trees

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:43
+
+  struct Roles {
+    static constexpr NodeRole eof = 1;
----------------
ilya-biryukov wrote:
> 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.
> 
I think we misunderstood each other here... I think this is fairly important, and that we'd agreed on it in offline discussion. Didn't mean to leave it as an optional comment.

I'd be very surprised if 256 were too limiting. Indeed most nodes will have children, but most of them will not have unique roles. (And I would be surprised if we have 200 node types, but maybe not that surprised...).

If there's a more fundamental objection to merging these, I'd like to find some agreement before going further.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61637