This is an archive of the discontinued LLVM Phabricator instance.

[Syntax] Build declarator nodes
ClosedPublic

Authored by hlopko on Mar 16 2020, 3:21 AM.

Details

Summary

Copy of https://reviews.llvm.org/D72089 with Ilya's permission. See
https://reviews.llvm.org/D72089 for the first batch of comments.

Diff Detail

Event Timeline

hlopko created this revision.Mar 16 2020, 3:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2020, 3:21 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
hlopko retitled this revision from Build declarator nodes to [Syntax] Build declarator nodes.Mar 16 2020, 4:15 AM
gribozavr2 added inline comments.Mar 16 2020, 7:54 AM
clang/include/clang/Tooling/Syntax/Nodes.h
519

... and a trailing return type, if the function has one.

522

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.

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.

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

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;
const int X::* const c;

Also for member functions:

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

hlopko updated this revision to Diff 250574.Mar 16 2020, 8:48 AM
hlopko marked 8 inline comments as done.

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;
int (X::*d)(int param);

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

gribozavr2 accepted this revision.Mar 16 2020, 9:43 AM
This revision is now accepted and ready to land.Mar 16 2020, 9:43 AM
hlopko updated this revision to Diff 250591.Mar 16 2020, 10:14 AM

Fix clang tidy warning.

gribozavr2 accepted this revision.Mar 16 2020, 11:15 AM

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;

This revision was automatically updated to reflect the committed changes.