Page MenuHomePhabricator

Migrate Declarators to use the List API
ClosedPublic

Authored by eduucaldas on Sep 28 2020, 4:56 AM.

Details

Summary

After this change all nodes that have a delimited-list are using the List API.

Implementation details:
Let's look at a declaration with multiple declarators:
int a, b;
To generate a declarator list node we need to have the range of declarators: a, b. However, the ClangAST actually stores them as separate declarations:
int a ;
int b;
To solve that we mark the declarators on each separate declaration in the ClangAST and then for the final declarator int b, we shrink its range to fit to the already marked declarators.

Diff Detail

Unit TestsFailed

TimeTest
50 mslinux > Clang-Unit.Tooling/Syntax/_/SyntaxTests::SyntaxTreeTests/BuildSyntaxTreeTest.ArrayDeclarator_Multidimensional/0
Note: Google Test filter = SyntaxTreeTests/BuildSyntaxTreeTest.ArrayDeclarator_Multidimensional/0 [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
50 mslinux > Clang-Unit.Tooling/Syntax/_/SyntaxTests::SyntaxTreeTests/BuildSyntaxTreeTest.ArrayDeclarator_Multidimensional/1
Note: Google Test filter = SyntaxTreeTests/BuildSyntaxTreeTest.ArrayDeclarator_Multidimensional/1 [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
50 mslinux > Clang-Unit.Tooling/Syntax/_/SyntaxTests::SyntaxTreeTests/BuildSyntaxTreeTest.ArrayDeclarator_Multidimensional/10
Note: Google Test filter = SyntaxTreeTests/BuildSyntaxTreeTest.ArrayDeclarator_Multidimensional/10 [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
40 mslinux > Clang-Unit.Tooling/Syntax/_/SyntaxTests::SyntaxTreeTests/BuildSyntaxTreeTest.ArrayDeclarator_Multidimensional/11
Note: Google Test filter = SyntaxTreeTests/BuildSyntaxTreeTest.ArrayDeclarator_Multidimensional/11 [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
40 mslinux > Clang-Unit.Tooling/Syntax/_/SyntaxTests::SyntaxTreeTests/BuildSyntaxTreeTest.ArrayDeclarator_Multidimensional/12
Note: Google Test filter = SyntaxTreeTests/BuildSyntaxTreeTest.ArrayDeclarator_Multidimensional/12 [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
View Full Test Results (752 Failed)

Event Timeline

eduucaldas created this revision.Sep 28 2020, 4:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2020, 4:56 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
eduucaldas requested review of this revision.Sep 28 2020, 4:56 AM
eduucaldas edited the summary of this revision. (Show Details)Sep 28 2020, 4:59 AM
eduucaldas added inline comments.
clang/lib/Tooling/Syntax/BuildTree.cpp
594

WDYT about the naming?

clang/unittests/Tooling/Syntax/BuildTreeTest.cpp
2932

This sounds weird and is probably the reason why SO many tests failed.

According to the grammar a function definition/declaration might only have one declarator:

function-definition:
attribute-specifier-seq_opt decl-specifier-seq_opt declarator virt-specifier-seq_opt function-body
...

But our implementation calls processDeclaratorAndDeclaration for DeclaratorDecl, which englobes FunctionDecl.

I also noticed that we seem to have quite extensive support for declarations even rarer ones:
StaticAssertDeclaration, LinkageSpecificationDeclaration ... Moreover, their names and definitions seem to adequately follow the grammar. However we don't have a FunctionDefinition as defined in the grammar. Was grouping FunctionDefintion and FunctionDeclaration as SimpleDeclaration a conscious choice? (I looked quickly at commits that added processDeclaratorAndDeclaration but couldn't find any clue)

gribozavr2 added inline comments.Sep 28 2020, 12:50 PM
clang/lib/Tooling/Syntax/BuildTree.cpp
593
594

Very nice name!

clang/unittests/Tooling/Syntax/BuildTreeTest.cpp
2932

It seemed reasonable for uniformity. However, I can definitely see an argument for defining a special kind of tree node for them, especially now that if we use SimpleDeclaration for functions we would always get a 1-element list wrapper.

On the other hand, the following is a valid declaration:

int x, f(int);

Wouldn't it be weird if function definitions and declarations were modeled differently? I understand they are different in the C++ grammar, but I think it would be at least a bit surprising for users.

eduucaldas marked 3 inline comments as done.Sep 29 2020, 1:23 AM
eduucaldas added inline comments.
clang/unittests/Tooling/Syntax/BuildTreeTest.cpp
2932

Oh my god... that hurts my eyes...

I don't know what is more weird to be fair. In my view unifying both seems like a similar decision as unifying expressions and statements under the same ClangAST node because:

  1. I think they have other syntactic differences.
  2. I think generally people see declarations and definitions as different things. Whereas having a list in the definition of a function is completely unexpected.

Looking back at the grammar for declarations I think we would also benefit of trying to simplify it before mapping it to syntax trees. Although I would love to do that, I'm afraid there's too much on my plate already.

Should I leave a FIXME regarding this and adapt the other tests? Or do you wanna discuss this more deeply? Both options are fine for me :)

eduucaldas marked an inline comment as done.

Improve comment

Update tests

gribozavr2 accepted this revision.Sep 29 2020, 11:00 AM
This revision is now accepted and ready to land.Sep 29 2020, 11:00 AM
This revision was automatically updated to reflect the committed changes.