[PATCH] D90659: [Syntax] Tablegen Sequence classes. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.td:123
+    Role<"CloseParen", Token<"r_paren">>,
+  ];
+}
----------------
gribozavr2 wrote:
> The reason why in my prototype the grammar rules are separate is to allow expressing rules that depend on each other in a circular fashion. Seems like you avoided the problem right now by removing the need for Expression to refer to all subclasses, but it would come back, in, for example, LambdaExpression -- which is an expression that needs to refer to CompoundStatement that is defined later in this file, while CompoundStatement will need to refer to Expression. Maybe there is also a way to break circularity there by careful ordering -- but we would be mixing the order of expressions and statements.
In the current setup, there's a simple solution to the ordering constraints: define all the Alternatives first. It's not ideal, but I think less confusing than having separate defs for rule vs nodes.

OTOH if we want Alternatives to have down-edges later (which I think would make sense if we use variants rather than subclassing) then we'll definitely run into cycles and will need to break them by splitting out the rules.
(Or if we have cycles that don't involve Alternatives, and don't want to introduce artificial ones).

I think we probably will hit this case, but would like to punt on it a little to see whether it makes sense to break loops only where needed or fully separate declaration/definition.


================
Comment at: clang/include/clang/Tooling/Syntax/Syntax.td:70
+//
+// Each child is characterized by a unique role and an allowed base type.
+// The role sequence and role/type match are enforced invariants of the class.
----------------
gribozavr2 wrote:
> 
Hmm, I don't like swapping the order here - the role is both more important and written first.
I think the issue is the scope of "unique" is ambiguous, I'll try to fix that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90659