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

Eduardo Caldas via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 20 06:01:49 PDT 2020


eduucaldas marked an inline comment as done.
eduucaldas added inline comments.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86227



More information about the cfe-commits mailing list