Page MenuHomePhabricator

[Syntax] Build declarator nodes
Needs ReviewPublic

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



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

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).


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


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


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.


+ 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.)


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.


Similarly, I think it should be an implementation comment.


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.


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].


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?


Looks like this patch was not updated for (because we're not passing the AST node to markChild)?


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


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.


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;