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