gribozavr2 added inline comments.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:395
}
+ /// FIXME: use custom iterator instead of 'vector'.
+ std::vector<syntax::SimpleDeclarator *> declarators();
----------------
s/iterator/container/ ?
Also unclear why a custom one should be used.
I also think it should be an implementation comment (in the .cc file).
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:491
+/// Array size specified inside a declarator.
+/// E.g. `[10]` in `int a[10]`.
+class ArraySubscript final : public Tree {
----------------
Also `[static 10]` in `void f(int xs[static 10]);`
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:500
+ syntax::Expression *sizeExpression();
+ syntax::Leaf *rbracket();
+};
----------------
+ TODO: add an accessor for the "static" keyword.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:503
+
+/// Trailing return type inside parameter list, starting from the arrow token.
+/// E.g. `-> int***`.
----------------
s/inside parameter list/after the parameter list/ ?
s/starting from/starting with/ or "including the arrow token" to emphasize that
the arrow is included.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:512
+ syntax::Leaf *arrow();
+ syntax::SimpleDeclarator *declarator();
+};
----------------
+ TODO: add accessors for specifiers.
Or we could add a syntax node for the "type-id" construct -- it seems like it
will be useful in other places that require exactly one type, like the argument
of sizeof, type argument of static_cast etc. (Also could be a TODO, this patch
is pretty long.)
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:515
+
+/// Parameter list for a function type.
+class ParametersAndQualifiers final : public Tree {
----------------
Would be great to show an example of what sort of qualifiers we're talking
about here (trailing "const", "volatile", "&", "&&", right?) What about
"noexcept"? Would be also good to say that "override" does not belong here.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:524
+ /// FIXME: use custom iterator instead of 'vector'.
+ std::vector<syntax::SimpleDeclaration *> parameters();
+ syntax::Leaf *rparen();
----------------
Similarly, I think it should be an implementation comment.
================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:53
+namespace {
+struct GetStartLoc : TypeLocVisitor<GetStartLoc, SourceLocation> {
+ SourceLocation VisitParenTypeLoc(ParenTypeLoc T) {
----------------
A brief comment about why this is necessary (and why it works) would be
appreciated. Just remind the reader that type locs are stored inside-out, and
that the start location in the source order would be on the innermost node.
================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:119
+ SourceLocation Start = GetStartLoc().Visit(T);
+ SourceLocation End = T.getSourceRange().getEnd();
+ assert(End.isValid());
----------------
Why don't we need a similar visitor to get the end location? I think we have a
similar inside-out problem with `int x[10][20]`.
================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:306
auto BeginExecuted = DelayedFolds.lower_bound(Tokens.begin());
- auto It = BeginExecuted;
- for (; It != DelayedFolds.end() && It->second.End <= Tokens.end(); ++It)
+ auto EndExecuted = BeginExecuted;
+ for (; EndExecuted != DelayedFolds.end() &&
----------------
The past tense in Executed threw me off -- I thought that we already executed
these folds, and now doing them again for some reason.
Consider `BeginFolds` / `EndFolds`?
================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:483
+ Builder.foldNode(Tokens, new (allocator()) syntax::SimpleDeclarator);
+ Builder.markChild(Tokens,
syntax::NodeRole::SimpleDeclaration_declarator);
+ }
----------------
Looks like this patch was not updated for https://reviews.llvm.org/D72446
(because we're not passing the AST node to markChild)?
================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:908
+ {R"cpp(
+static_assert(true, "message");
+static_assert(true);
----------------
I think we already have one of these tests for static_assert.
================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:931
+ )txt"},
+ // Array subscripts in declarators.
+ {R"cpp(
----------------
Now that this test is going over 1000 lines, I'd really appreciate splitting it
into multiple files, each focusing on one aspect (e.g., one file for testing
declarators). Could be a follow-up change.
================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:1210
+ `-;
+ )txt"},
};
----------------
A few complex tests that combine multiple declarators would be nice (especially
to test that the delayed fold logic composes correctly).
```
char (*(*x[10])(short a, int b))[20];
char (*(*x[10][20])(short a, int b))[30][40];
void x(char a, short (*b)(int));
void x(char a, short (*b)(int), long (**c)(long long));
void (*x(char a, short (*b)(int)))(long);
```
Also qualifiers -- or are they not implemented yet?
```
int * const * volatile x;
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72089/new/
https://reviews.llvm.org/D72089
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
