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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 accepted this revision.
gribozavr2 added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:346
+/// static_assert(<condition>, <message>)
+/// static_assert(<condition>)
+class StaticAssertDeclaration final : public Declaration {
----------------
Why no semicolon, here and in other newly-added comments below? Seems like these syntax nodes cover the semicolon as implemented.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:378
+
+/// namespace <name> { <decls> }
+class NamespaceDefinition final : public Declaration {
----------------
Isn't it a "nested name specifier" since C++17?


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


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:517
+      {R"cpp(
+namespace a { namespace b {} }
+namespace {}
----------------
Also add `namespace a::b {}` ?


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:692
+static_assert(true, "message");
+static_assert(true);
+    )cpp",
----------------
Duplicate test? (There's one above that's exactly like this.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70856