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