[PATCH] D72089: [Syntax] Build declarator nodes

[Dmitri Gribenko via Phabricator via cfe-commits]

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