Copy of https://reviews.llvm.org/D72089 with Ilya's permission. See
https://reviews.llvm.org/D72089 for the first batch of comments.
Details
- Reviewers
gribozavr2 - Commits
- rG7d382dcd46a1: [Syntax] Build declarator nodes
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/include/clang/Tooling/Syntax/Nodes.h | ||
---|---|---|
519 | ... and a trailing return type, if the function has one. | |
522 | I meant: int foo() volatile; Of course parameters can be cv and ref-qualified, but that's not the interesting part here. What's interesting is function qualifiers. | |
526 | Would you mind adding these examples to tests? | |
544 | 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. | |
clang/lib/Tooling/Syntax/BuildTree.cpp | ||
53 | s/of the Decl/of the declarator/ | |
61 | I don't think LLVM does (!) type of ASCII art. """ | |
106 | 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? | |
clang/unittests/Tooling/Syntax/TreeTest.cpp | ||
1282 | Please also add tests with qualifiers. const int X::* b; Also for member functions: int (X::*d)(int param); |
Resolving comments.
clang/include/clang/Tooling/Syntax/Nodes.h | ||
---|---|---|
522 | Rewrote the comment, added tests. | |
526 | We already have them (see "Exception specification in parameter lists" and "Trailing return type in parameter lists"). | |
544 | 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. | |
clang/lib/Tooling/Syntax/BuildTree.cpp | ||
61 | Ack, I saw some usages of (!) so was using it too :) Removed in the whole file. Updated the comment. | |
106 | Noted, will address in a separate patch. | |
clang/unittests/Tooling/Syntax/TreeTest.cpp | ||
1282 | These 2 tests break asserts, will fix in a separate patch. const int X::* const c; Added a test for const int X::* b; into this one. |
BTW, would be also nice to have tests for trailing return types not at the top level -- in a follow-up:
auto x(char a, auto (*b)(int) -> short) -> void;
... and a trailing return type, if the function has one.