[PATCH] D76220: [Syntax] Build declarator nodes

[Marcel Hlopko via Phabricator via cfe-commits]

hlopko added inline comments.


================
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;`
----------------
gribozavr2 wrote:
> 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.
Rewrote the comment, added tests.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:526
+///  `() noexcept` in `int foo() noexcept;`
+///  `() throw()` in `int foo() throw();`
+///
----------------
gribozavr2 wrote:
> Would you mind adding these examples to tests?
We already have them (see "Exception specification in parameter lists" and "Trailing return type in parameter lists").


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:544
+/// E.g. `X::*` in `int X::* a = 0;`
+class MemberPointer final : public Tree {
+public:
----------------
gribozavr2 wrote:
> 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.
Yeah I agree, and even though not very helpful, it looks more polished to me, so I'll add upload a separate patch with that.


================
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
----------------
gribozavr2 wrote:
> 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, ...
> """
Ack, I saw some usages of (!) so was using it too :) Removed in the whole file.

Updated the comment.


================
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);
----------------
gribozavr2 wrote:
> 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?
Noted, will address in a separate patch.


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:1282
+struct X {};
+int X::* a;
+    )cpp",
----------------
gribozavr2 wrote:
> Please also add tests with qualifiers.
> 
> `const int X::* b;`
> `const int X::* const c;`
> 
> Also for member functions:
> 
> `int (X::*d)(int param);`
These 2 tests break asserts, will fix in a separate patch.

const int X::* const c;
int (X::*d)(int param);


Added a test for `const int X::* b;` into this one.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76220