[PATCH] D70856: [Syntax] Build nodes for simple cases of top level declarations

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

Updated version LGTM, thanks!



================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:346
+/// static_assert(<condition>, <message>)
+/// static_assert(<condition>)
+class StaticAssertDeclaration final : public Declaration {
----------------
ilya-biryukov wrote:
> gribozavr2 wrote:
> > Why no semicolon, here and in other newly-added comments below? Seems like these syntax nodes cover the semicolon as implemented.
> I was torn on this...
> In the statement position, this semicolon is not consumed by declarations (instead, it's consumed by `DeclarationStatement`). However, it **is** consumed at the namespace and TU level.
> 
> I've decided to punt on this until we add accessors for the semicolon and add a comment to the corresponding accessors, explaining the percularities of its placement.
> 
> Decided to keep it out from the comment, since it's not present **sometimes**.
> 
> Don't have a strong opinion here, can add it back
It is fine to punt until later.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:378
+
+/// namespace <name> { <decls> }
+class NamespaceDefinition final : public Declaration {
----------------
ilya-biryukov wrote:
> gribozavr2 wrote:
> > Isn't it a "nested name specifier" since C++17?
> nested-name-specifier is a qualifier
> it's actually something like `<nested-name-qualifier>? <identifier>`. Which is quite verbose, so decided decided to go with `<name>` to keep it small for now.
> 
> May have to change to something more suitable when we actually start building syntax trees for names.
> 
> Does keeping `<name>` make sense for now? Do you think we should be more precise from the start?
> 
> Happy to go in either direction, actually.
"name" != "identifier" so it is technically correct. Once we have accessors, they should have comments about what the actual options for the name are, and with that, I don't mind having just "name" here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70856