Add support for DeclRefExpr in SyntaxTree, by generating IdExpressions

[eduucaldas]

clang/unittests/Tooling/Syntax/TreeTest.cpp

829

Perhaps we shouldn't differ between specifiers as that is semantical information

[eduucaldas]

.

clang/unittests/Tooling/Syntax/TreeTest.cpp

829

Followed the grammar, but added specifier in the end as we also hold the :: and not only the names

[gribozavr2]

clang/unittests/Tooling/Syntax/TreeTest.cpp

815

This part does not seem to appear in the source.

[eduucaldas]

cleanup for upstreaming

[gribozavr2]

clang/include/clang/Tooling/Syntax/Nodes.h

190 ↗

(On Diff #269129)

Should there be getters for various parts of the specifier?

203 ↗

(On Diff #269129)

Could you add more details? Feel free to even quote the relevant grammar bits.

211 ↗

(On Diff #269129)

unqualified-id in the grammar can be more than one token.

212 ↗

(On Diff #269129)

Could you swap the order of getters to match the source order?

clang/lib/Tooling/Syntax/BuildTree.cpp

618 ↗

(On Diff #269129)

Do we need to mark the role if it is unknown?

clang/unittests/Tooling/Syntax/TreeTest.cpp

751

Could you add more tests even if they fail today due to missing implementation?

a::b::operator-(a::b::S())
a::b::S2<int>::f()

etc. Basically cover the whole grammar in tests, if possible.

[eduucaldas]

clang/lib/Tooling/Syntax/BuildTree.cpp

618 ↗

(On Diff #269129)

Thanks, that had slipped through, sorry for that

[eduucaldas]

This comment was removed by eduucaldas.

[eduucaldas]

Answering simple comments

[gribozavr2]

clang/include/clang/Tooling/Syntax/Nodes.h

209 ↗

(On Diff #269423)

Please add a TODO for the accessor for the 'template' keyword (and a test that has that keyword).

[eduucaldas]

• cleanup for upstreaming
• better coverage for unqualified-id
• Better coverage for qualified-id
• Finish implementing unqualified-id.

[eduucaldas]

clang/include/clang/Tooling/Syntax/Nodes.h

209 ↗

(On Diff #269423)

I can implement accessor. But I couldn't write a test with this template keyword that uses DeclRefExpr. This is the test I came up with, note that both expressions are represented as DependentScopeDeclRefExpr

216 ↗

(On Diff #269423)

Just by looking at code it is impossible to know if the accessed thing is optional or not.
IdExpression ALWAYS has a UnqualifiedId
IdExpression MAY have a NestedNameSpecifier
This same issue repeats in other places

190 ↗

(On Diff #269129)

I think not.

[eduucaldas]

Fix mistake on getting SourceRange for template-id

[gribozavr2]

clang/include/clang/Tooling/Syntax/Nodes.h

187 ↗

(On Diff #271620)

Could you provide examples or a grammar quote in the comment?

207 ↗

(On Diff #271620)

"the size in std::vector<int>::size"

214 ↗

(On Diff #271620)

Could you add TODOs about adding accessors?

clang/unittests/Tooling/Syntax/TreeTest.cpp

696

Could you split off the parts of this test that don't depend on C++11?

979

"WithTemplateKeyword"

1097

"QualifiedIdDecltype" for consistency with other tests.

1105

Indent -2?

1108

Could you indent the second line?

[eduucaldas]

Aswering comments

[eduucaldas]

clang/include/clang/Tooling/Syntax/Nodes.h

214 ↗

(On Diff #271620)

Pospone to interface discussion with Yitzhak

190 ↗

(On Diff #269129)

Pospone to interface discussion with Yitzhak

clang/lib/Tooling/Syntax/Nodes.cpp

178 ↗

(On Diff #271744)

I was thinking and our problem is bigger than not testing for NodeRoles, we don't test accessors at all!

182 ↗

(On Diff #271744)

This should be an assert for now, as if that is not true it would be a logic problem. Same reasoning applies to CompoundStatement

[gribozavr2]

clang/lib/Tooling/Syntax/Nodes.cpp

182 ↗

(On Diff #271744)

Yep, feel free to change it to an assert.

[eduucaldas]

change if -> asserts

[MaskRay]

@eduucaldas

Hi, you can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (I have updated my script to use --date=now (setting author date to committer date))

https://reviews.llvm.org/D80978 contains a git pre-push hook to automate this.

clang/unittests/Tooling/Syntax/TreeTest.cpp

`-CompoundStatement
     |   | | `-test
     |   | |-(
     |   | `-)
     |   `-;
     `-}
 )txt"));
 }
 
+TEST_P(SyntaxTreeTest, UnqualifiedId) {
+  if (!GetParam().isCXX()) {
+    return;
+  }
+  EXPECT_TRUE(treeDumpEqual(
+      R"cpp(
+void test(int b) {
+  int a;
+  a = b;
+}
+)cpp",
+      R"txt(
+*: TranslationUnit
+`-SimpleDeclaration
+  |-void
+  |-SimpleDeclarator
+  | |-test
+  | `-ParametersAndQualifiers
+  |   |-(
+  |   |-SimpleDeclaration
+  |   | |-int
+  |   | `-SimpleDeclarator
+  |   |   `-b
+  |   `-)
+  `-CompoundStatement
+    |-{
+    |-DeclarationStatement
+    | |-SimpleDeclaration
+    | | |-int
+    | | `-SimpleDeclarator
+    | |   `-a
+    | `-;
+    |-ExpressionStatement
+    | |-BinaryOperatorExpression
+    | | |-IdExpression
+    | | | `-UnqualifiedId
+    | | |   `-a
+    | | |-=
+    | | `-IdExpression
+    | |   `-UnqualifiedId
+    | |     `-b
+    | `-;
+    `-}
+)txt"));
+}
+
+TEST_P(SyntaxTreeTest, QualifiedId) {
+  if (!GetParam().isCXX()) {
+    return;
+  }
+  EXPECT_TRUE(treeDumpEqual(
+      R"cpp(
+namespace a {
+  namespace b {
+    struct S {
+      int i;
+      static void f(){}
+    };
+  }
+}
+void test(int b) {
+  ::a::b::S::f();
+}
+)cpp",
+      R"txt(
+*: TranslationUnit
+|-NamespaceDefinition
+| |-namespace
+| |-a
+| |-{
+| |-NamespaceDefinition
+| | |-namespace
+| | |-b
+| | |-{
+| | |-SimpleDeclaration
+| | | |-struct
+| | | |-S
+| | | |-{
+| | | |-SimpleDeclaration
+| | | | |-int
+| | | | |-SimpleDeclarator
+| | | | | `-i
+| | | | `-;
+| | | |-SimpleDeclaration
+| | | | |-static
+| | | | |-void
+| | | | |-SimpleDeclarator
+| | | | | |-f
+| | | | | `-ParametersAndQualifiers
+| | | | |   |-(
+| | | | |   `-)
+| | | | `-CompoundStatement
+| | | |   |-{
+| | | |   `-}
+| | | |-}
+| | | `-;
+| | `-}
+| `-}
+`-SimpleDeclaration
+  |-void
+  |-SimpleDeclarator
+  | |-test
+  | `-ParametersAndQualifiers
+  |   |-(
+  |   |-SimpleDeclaration
+  |   | |-int
+  |   | `-SimpleDeclarator
+  |   |   `-b
+  |   `-)
+  `-CompoundStatement
+    |-{
+    |-DeclarationStatement
+    | |-SimpleDeclaration
+    | | |-a
+    | | |-::
+    | | |-b
+    | | |-::
+    | | |-S
+    | | `-SimpleDeclarator
+    | |   `-UnknownExpression
+    | |     `-s
+    | `-;
+    |-ExpressionStatement
+    | |-UnknownExpression
+    | | |-IdExpression
+    | | | |-NameSpecifiers
+    | | | | |-GlobalNamespaceSpecifier
+    | | | | | `::
+    | | | | |-NamespaceSpecifier
+    | | | | | |-a
+    | | | | | `-::
+    | | | | |-NamespaceSpecifier
+    | | | | | |-b
+    | | | | | `-::
+    | | | | `-TypeSpecifier
+    | | | |   |-S
+    | | | |   `-::
+    | | | `-UnqualifiedId
+    | | |   `-f
+    | | |-(
+    | | `-)
+    | `-;
+    `-}
+)txt"));
+}
+
 TEST_P(SyntaxTreeTest, CxxNullPtrLiteral) {
   if (!GetParam().isCXX11OrLater()) {
     return;
   }
   EXPECT_TRUE(treeDumpEqual(
       R"cpp(
 void test() {
   nullptr;
`-SimpleDeclaration
   | `-ParametersAndQualifiers
   |   |-(
   |   `-)
   `-CompoundStatement
     |-{
     |-ExpressionStatement
     | |-IntegerLiteralExpression
     | | `-0b1100
     | `-;
     `-}
 )txt"));
 }
 
 TEST_P(SyntaxTreeTest, IntegerLiteralWithDigitSeparators) {
   if (!GetParam().isCXX14OrLater()) {
     return;
   }
`-CompoundStatement
     | | |---
     | | `-UnknownExpression
     | |   `-a
     | `-;
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
     | | |-++
     | | `-UnknownExpression
     | |   `-a
     | `-;
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
     | | |-~
     | | `-UnknownExpression
     | |   `-a
     | `-;
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
     | | |--
     | | `-UnknownExpression
     | |   `-a
     | `-;
     |-ExpressionStatement
     | |-PrefixUnaryOperatorExpression
     | | |-+
     | | `-UnknownExpression
     | |   `-a
     | `-;