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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:346
+/// static_assert(<condition>, <message>)
+/// static_assert(<condition>)
+class StaticAssertDeclaration final : public Declaration {
----------------
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


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:378
+
+/// namespace <name> { <decls> }
+class NamespaceDefinition final : public Declaration {
----------------
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.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:515
+    Builder.markExprChild(S->getAssertExpr(),
+                          syntax::NodeRole::StaticAssertDeclaration_condition);
+    Builder.foldNode(Builder.getRange(S),
----------------
gribozavr2 wrote:
> Why not also mark the message?
Good point. Done.

We don't have a node for `StringLiteral`, but we're returning it as `<expression>` for now.


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:517
+      {R"cpp(
+namespace a { namespace b {} }
+namespace {}
----------------
gribozavr2 wrote:
> Also add `namespace a::b {}` ?
Done. This was also broken, now fixed. Thanks for bringing this up.


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:692
+static_assert(true, "message");
+static_assert(true);
+    )cpp",
----------------
gribozavr2 wrote:
> Duplicate test? (There's one above that's exactly like this.)
Thanks! Totally accidental, did not notice this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70856