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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
