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