This is an archive of the discontinued LLVM Phabricator instance.

[SyntaxTree] Add support for `MemberExpression`
ClosedPublic

Authored by eduucaldas on Aug 19 2020, 9:45 AM.

Diff Detail

Event Timeline

eduucaldas created this revision.Aug 19 2020, 9:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2020, 9:45 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
eduucaldas requested review of this revision.Aug 19 2020, 9:45 AM
eduucaldas added inline comments.
clang/include/clang/Tooling/Syntax/Nodes.h
333

We could discuss how to model the implicit member expression, as it has a totally different syntax.

clang/unittests/Tooling/Syntax/BuildTreeTest.cpp
1769–1771

Not much progress trying to use other ignores. Perhaps we can treat this properly when adding support for CXXConstructExpr

2086–2087

I'm experimenting with that now, but anyways, I think this should go to the patch,

[SyntaxTree] Add support to CXXThisExpr

gribozavr2 added inline comments.Aug 20 2020, 2:01 AM
clang/include/clang/Tooling/Syntax/Nodes.h
180

Could you order new items in source order? object, access token, member.

333

I think the syntax tree should represent only the syntax that was actually present in the source code. IOW, implicit member expression from Clang AST should not map to a member expression in the syntax tree. The syntax tree should represent just the id-expression.

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

Please make unqualifiedId variable start with the uppercase character. Since it would clash with the type name, you can do TheUnqualifiedId, for example.

Same for idExpression below.

911

This code seems largely identical to WalkUpFromDeclRefExpr. (It is also somewhat difficult to track what is happening because we are creating three levels of nodes here.) Could we factor out the repeated code?

clang/unittests/Tooling/Syntax/BuildTreeTest.cpp
634–636
2085
2086–2087
2101
2133
2165

Please add tests that access static members through member expression syntax.

2165

Please add a test for variable templates:

struct S {
    template<typename T>
    static constexpr T x = 42;
};

void f(S s) {
    s.x<int>;
}

Implicit MemberExpr generates id-expression

eduucaldas marked 10 inline comments as done.

Answering comments

eduucaldas marked an inline comment as done.Aug 20 2020, 6:01 AM
eduucaldas added inline comments.
clang/lib/Tooling/Syntax/BuildTree.cpp
900

I just saw in the style guide the rules for casing.

I didn't expect this to exist because it doesn't seem to be obeyed ^^. For instance WalkUpFrom* starts with Upper-case.

I'll follow-up with a patch fixing any style problems of this nature.

911

I agree.
However there are some things that change between semantic nodes that produce an IdExpression.

  • We may or not have a link from IdExpression to the AST.
  • The SourceRange for the UnqualifiedId is obtained in an ad-hoc manner.

Taking into these variables we could write a function buildIdExpression

  1. IdExpression* buildIdExpression(Expr* E, SourceRange UnqualifiedIdLoc, bool shouldLinkToAST);
  2. IdExpression* buildIdExpression(SourceRange QualifierLoc, SourceRange TemplateKwLoc, SourceRange UnqualifiedIdLoc, ASTPtr link);

The first option takes into account only the differences that we perceived until now.
The second option provides a more general approach.

eduucaldas marked an inline comment as done.Aug 20 2020, 6:30 AM
gribozavr2 accepted this revision.Aug 20 2020, 6:36 AM
gribozavr2 added inline comments.
clang/lib/Tooling/Syntax/BuildTree.cpp
911

I'd prefer (2), it is less subtle and more explicit.

clang/unittests/Tooling/Syntax/BuildTreeTest.cpp
1999
This revision is now accepted and ready to land.Aug 20 2020, 6:36 AM
eduucaldas marked 2 inline comments as done.

Unify logic for generating id-expression

This revision was automatically updated to reflect the committed changes.