Page MenuHomePhabricator

[Syntax] Build declarator nodes
Needs ReviewPublic

Authored by ilya-biryukov on Thu, Jan 2, 8:32 AM.

Details

Reviewers
gribozavr2
Summary

They cover part of types and names for some declarations, including
common cases like variables, functions, etc.

See the comment of Declarator for more details.

Diff Detail

Event Timeline

ilya-biryukov created this revision.Thu, Jan 2, 8:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jan 2, 8:32 AM

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

  • Rebase, get rid of accidental changes with TemplateDeclaration

Unit tests: pass. 61848 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

gribozavr2 added inline comments.Thu, Jan 16, 7:05 AM
clang/include/clang/Tooling/Syntax/Nodes.h
395

s/iterator/container/ ?

Also unclear why a custom one should be used.

I also think it should be an implementation comment (in the .cc file).

491

Also [static 10] in void f(int xs[static 10]);

500

+ TODO: add an accessor for the "static" keyword.

503

s/inside parameter list/after the parameter list/ ?

s/starting from/starting with/ or "including the arrow token" to emphasize that the arrow is included.

512

+ TODO: add accessors for specifiers.

Or we could add a syntax node for the "type-id" construct -- it seems like it will be useful in other places that require exactly one type, like the argument of sizeof, type argument of static_cast etc. (Also could be a TODO, this patch is pretty long.)

515

Would be great to show an example of what sort of qualifiers we're talking about here (trailing "const", "volatile", "&", "&&", right?) What about "noexcept"? Would be also good to say that "override" does not belong here.

524

Similarly, I think it should be an implementation comment.

clang/lib/Tooling/Syntax/BuildTree.cpp
53

A brief comment about why this is necessary (and why it works) would be appreciated. Just remind the reader that type locs are stored inside-out, and that the start location in the source order would be on the innermost node.

119

Why don't we need a similar visitor to get the end location? I think we have a similar inside-out problem with int x[10][20].

306

The past tense in Executed threw me off -- I thought that we already executed these folds, and now doing them again for some reason.

Consider BeginFolds / EndFolds?

483

Looks like this patch was not updated for https://reviews.llvm.org/D72446 (because we're not passing the AST node to markChild)?

clang/unittests/Tooling/Syntax/TreeTest.cpp
908

I think we already have one of these tests for static_assert.

931

Now that this test is going over 1000 lines, I'd really appreciate splitting it into multiple files, each focusing on one aspect (e.g., one file for testing declarators). Could be a follow-up change.

1210

A few complex tests that combine multiple declarators would be nice (especially to test that the delayed fold logic composes correctly).

char (*(*x[10])(short a, int b))[20];
char (*(*x[10][20])(short a, int b))[30][40];
void x(char a, short (*b)(int));
void x(char a, short (*b)(int), long (**c)(long long));
void (*x(char a, short (*b)(int)))(long);

Also qualifiers -- or are they not implemented yet?

int * const * volatile x;