[PATCH] D86227: [SyntaxTree] Add support for `MemberExpression`

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:180
+  MemberExpression_member,
+  MemberExpression_accessToken,
 };
----------------
Could you order new items in source order? object, access token, member.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:333
+///   expression .  template_opt id-expression
+///   id-expression
+/// e.g. `x.a`, `xp->a` or even just `a` when we have an implicit `this->`.
----------------
eduucaldas wrote:
> We could discuss how to model the implicit member expression, as it has a totally different syntax.
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.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:900
+
+    auto *unqualifiedId = new (allocator()) syntax::UnqualifiedId;
+    Builder.foldNode(Builder.getRange(S->getMemberLoc(), S->getEndLoc()),
----------------
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.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:911
+                                      S->getEndLoc()),
+                     idExpression, nullptr);
+
----------------
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?


================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:634-636
+  // FIXME: Remove the terminal `UnknownExpression` wrapping `s1` and `s2`. This
+  // `UnknownExpression` comes from a terminal `CXXConstructExpr` in the
+  // ClangAST. We need to ignore terminal implicit nodes.
----------------



================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:2085
+  int a;
+  int geta(){
+    // FIXME: Remove the terminal`UnknownExpression` wrapping `a`. This
----------------



================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:2086-2087
+  int geta(){
+    // FIXME: Remove the terminal`UnknownExpression` wrapping `a`. This
+    // `UnknownExpression` comes from a terminal implicit `CXXThisExpr`.
+    [[a]];
----------------
eduucaldas wrote:
> I'm experimenting with that now, but anyways, I think this should go to the patch, 
> > [SyntaxTree] Add support to `CXXThisExpr`



================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:2101
+
+TEST_P(SyntaxTreeTest, MemberExpression_Template) {
+  if (!GetParam().isCXX()) {
----------------



================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:2133
+
+TEST_P(SyntaxTreeTest, MemberExpression_TemplateWithTemplateKeyword) {
+  if (!GetParam().isCXX()) {
----------------



================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:2165
+}
+
+TEST_P(SyntaxTreeTest, MemberExpression_WithQualifier) {
----------------
Please add tests that access static members through member expression syntax.



================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:2165
+}
+
+TEST_P(SyntaxTreeTest, MemberExpression_WithQualifier) {
----------------
gribozavr2 wrote:
> Please add tests that access static members through member expression syntax.
> 
Please add a test for variable templates:

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

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86227/new/

https://reviews.llvm.org/D86227