[PATCH] D72089: [Syntax] Build declarator nodes
Marcel Hlopko via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 16 03:53:17 PDT 2020
hlopko added a comment.
I'm taking over of this patch (with a kind permission from Ilya :) I've replied to comments here, but let's continue the review over at https://reviews.llvm.org/D76220. Thanks :)
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:395
}
+ /// FIXME: use custom iterator instead of 'vector'.
+ std::vector<syntax::SimpleDeclarator *> declarators();
----------------
gribozavr2 wrote:
> 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).
Waiting for reply from Ilya offline, in the meantime I'm keeping the comment as is.
================
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 {
----------------
gribozavr2 wrote:
> Also `[static 10]` in `void f(int xs[static 10]);`
Added example into the comment + added a test verifying it's handled correctly.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:500
+ syntax::Expression *sizeExpression();
+ syntax::Leaf *rbracket();
+};
----------------
gribozavr2 wrote:
> + TODO: add an accessor for the "static" keyword.
Added a TODO.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:503
+
+/// Trailing return type inside parameter list, starting from the arrow token.
+/// E.g. `-> int***`.
----------------
gribozavr2 wrote:
> 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.
Done.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:512
+ syntax::Leaf *arrow();
+ syntax::SimpleDeclarator *declarator();
+};
----------------
gribozavr2 wrote:
> + 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.)
Added a TODO (also mentioning type-id node)
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:515
+
+/// Parameter list for a function type.
+class ParametersAndQualifiers final : public Tree {
----------------
gribozavr2 wrote:
> 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.
Added a comment (and added tests where there was no coverage):
+/// E.g.:
+/// `(volatile int a)` in `int foo(volatile int a);`
+/// `(int&& a)` in `int foo(int&& a);`
+/// `() -> int` in `auto foo() -> int;`
+/// `() const` in `int foo() const;`
+/// `() noexcept` in `int foo() noexcept;`
+/// `() throw()` in `int foo() throw();`
+///
+/// (!) override doesn't 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();
----------------
gribozavr2 wrote:
> Similarly, I think it should be an implementation comment.
Will handle once I hear from Ilya.
================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:53
+namespace {
+struct GetStartLoc : TypeLocVisitor<GetStartLoc, SourceLocation> {
+ SourceLocation VisitParenTypeLoc(ParenTypeLoc T) {
----------------
gribozavr2 wrote:
> 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.
Added:
53 /// Get start location of the Decl from the TypeLoc.
54 /// E.g.:
55 /// loc of `(` in `int (a)`
56 /// loc of `*` in `int *(a)`
57 /// loc of the first `(` in `int (*a)(int)`
58 /// loc of the `*` in `int *(a)(int)`
59 /// loc of the first `*` in `const int *const *volatile a;`
60 ///
61 /// (!) TypeLocs are stored inside out (in the example above `*volatile` is
62 /// the TypeLoc returned by `Decl.getTypeSourceInfo()`, and `*const` is
63 /// what `.getPointeeLoc()` returns.
================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:119
+ SourceLocation Start = GetStartLoc().Visit(T);
+ SourceLocation End = T.getSourceRange().getEnd();
+ assert(End.isValid());
----------------
gribozavr2 wrote:
> 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]`.
Could you elaborate? I added a test for `int a[1][2][3] and it seems to be working correctly?
================
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() &&
----------------
gribozavr2 wrote:
> 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`?
Used 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);
+ }
----------------
gribozavr2 wrote:
> Looks like this patch was not updated for https://reviews.llvm.org/D72446 (because we're not passing the AST node to markChild)?
I plan to submit declarator nodes patch, then template nodes patch, then https://reviews.llvm.org/D72446. So this comment will be handled by that patch.
================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:908
+ {R"cpp(
+static_assert(true, "message");
+static_assert(true);
----------------
gribozavr2 wrote:
> I think we already have one of these tests for static_assert.
Removed once copy.
================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:931
+ )txt"},
+ // Array subscripts in declarators.
+ {R"cpp(
----------------
gribozavr2 wrote:
> 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.
Will happily do in a separate patch.
================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:1210
+ `-;
+ )txt"},
};
----------------
gribozavr2 wrote:
> 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;
> ```
>
Some of these tests actually crash, so I'll debug/fix in a separate patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72089/new/
https://reviews.llvm.org/D72089
More information about the cfe-commits
mailing list