[PATCH] D76220: [Syntax] Build declarator nodes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:519
+
+/// Parameter list for a function type.
+/// E.g.:
----------------
... and a trailing return type, if the function has one.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:522
+///  `(volatile int a)` in `int foo(volatile int a);`
+///  `(int&& a)` in `int foo(int&& a);`
+///  `() -> int` in `auto foo() -> int;`
----------------
I meant:

`int foo() volatile;`
`int foo() &&;`

Of course parameters can be cv and ref-qualified, but that's not the interesting part here. What's interesting is function qualifiers.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:526
+///  `() noexcept` in `int foo() noexcept;`
+///  `() throw()` in `int foo() throw();`
+///
----------------
Would you mind adding these examples to tests?


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:544
+/// E.g. `X::*` in `int X::* a = 0;`
+class MemberPointer final : public Tree {
+public:
----------------
Seems a bit weird that we have a separate node for member pointers, array subscripts, but not for regular pointers.

I think the rationale behind it is that since we are not building a tree structure out of declarators, the only token under a regular pointer declarator would be a star, so creating that node would not be very helpful.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:53
+namespace {
+/// Get start location of the Decl from the TypeLoc.
+/// E.g.:
----------------
s/of the Decl/of the declarator/


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:61
+///
+///   (!) TypeLocs are stored inside out (in the example above `*volatile` is
+///       the TypeLoc returned by `Decl.getTypeSourceInfo()`, and `*const` is
----------------
I don't think LLVM does `(!)` type of ASCII art.

"""
It is non-trivial to get the start location because TypeLocs are stored inside out. In the example above, ...
"""


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:106
+    if (T.getTypePtr()->hasTrailingReturn())
+      return SourceLocation(); // avoid recursing into the suffix of declarator.
+    return VisitTypeLoc(T);
----------------
I suspect there might be an issue here with nested function declarators because we will avoid recursing not only into the suffix (which I think means the trailing return type), but also into parameters.

If it was not necessary to recurse into parameters, why would the return statement below do that?


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:1282
+struct X {};
+int X::* a;
+    )cpp",
----------------
Please also add tests with qualifiers.

`const int X::* b;`
`const int X::* const c;`

Also for member functions:

`int (X::*d)(int param);`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76220/new/

https://reviews.llvm.org/D76220