[cfe-commits] r44156 - in /cfe/trunk: Driver/RewriteTest.cpp Sema/SemaExpr.cpp include/clang/AST/Expr.h

[Chris Lattner]

>> I assume that this is similar to a C++ example like this:
>>
>> class foo {
>>  int x;
>>
>>  int bar() { return x; }
>> };
>>
>> Why is it more natural to refer to "x" as a field reference than as  
>> a declref of the FieldDecl?  I know that GCC explicitly does  
>> lowering when is parses (making this a field reference off the  
>> hidden this/self parameter), but that doesn't seem like the right  
>> choice for our ASTs.
>>
>> Thoughts?
>
> Chris,
>
> It's a little more complicated than the above example. Consider this  
> pattern from "Sketch"...with the exception of the "copy" variable,  
> all other variable references in this example are represented by  
> "ObjCIvarRefExpr" (_bounds, _isDrawingFill, etc.).
>
> - (id)copyWithZone:(NSZone *)zone {
>
>     SKTGraphic *copy = [[[self class] alloc] init];
>     copy->_bounds = _bounds;
>     copy->_isDrawingFill = _isDrawingFill;
>     copy->_fillColor = [_fillColor copy];
>     copy->_isDrawingStroke = _isDrawingStroke;
>     copy->_strokeColor = [_strokeColor copy];
>     copy->_strokeWidth = _strokeWidth;
>     return copy;
> }

Ok, thanks for the example!

> We never "lower" the ivar references. The AST reflects the code  
> above. For example, there are no synthesized "MemberExpr's" for the  
> ivar references on the right hand side of the assignment expressions  
> above. ObjCIvarRefExpr represents both patterns. The arrow for the  
> left-hand side ivar references is really part of the ObjC syntax  
> (since the type of "copy" is an interface, not a structure).

Okay, I'm sorry.  When I saw the patch, I thought the addition was to  
MemberExpr, I didn't realize it was ObjCIvarRefExpr.

> That said, I still think what we have can be improved. From my  
> perspective, ObjCIvarRefExpr could be improved to represent the  
> "common" case more naturally, where we don't need an "Expr *" (since  
> "self" is assumed). This would both save space and remove improve  
> unnecessary code from Sema::ActOnIdentifierExpr(). Here's what I am  
> talking about...
>
>       if (CurMethodDecl) {
>         ObjcInterfaceDecl *IFace = CurMethodDecl->getClassInterface();
>         ObjcInterfaceDecl *clsDeclared;
>         if (ObjcIvarDecl *IV = IFace->lookupInstanceVariable(&II,  
> clsDeclared)) {
>           IdentifierInfo &II = Context.Idents.get("self"); //  
> unnecessary.
>           ExprResult SelfExpr = ActOnIdentifierExpr(S, Loc, II,  
> false); // ditto.
>           return new ObjCIvarRefExpr(IV, IV->getType(), Loc,
>                        static_cast<Expr*>(SelfExpr.Val), true, true);
>         }
>       }
>
> ...would become....
>
>       if (CurMethodDecl) {
>         ObjcInterfaceDecl *IFace = CurMethodDecl->getClassInterface();
>         ObjcInterfaceDecl *clsDeclared;
>         if (ObjcIvarDecl *IV = IFace->lookupInstanceVariable(&II,  
> clsDeclared))
>           return new ObjCIvarRefExpr(IV, IV->getType(), Loc);
>       }
>
> For now, I suggest we make the change above and have "Base" be null  
> for normal ivar references. As extra credit, we could factor this  
> out into two classes for each of the above cases.

I think this is a great step in the right direction.  However, what is  
the advantage of using ObjCIvarRefExpr over DeclRefExpr?  With the  
proposed code, you are capturing exactly the same information as  
DeclRefExpr would.  Just replace:

   return new ObjCIvarRefExpr(IV, IV->getType(), Loc);
with:
   return new DeclRefExpr(IV, IV->getType(), Loc);

Also, I consider the rewriter to be somewhat orthogonal to this  
discussion.  This is really about getting the ASTs right.  Getting the  
ASTs right requires being aware of the clients, but simplifying the  
ASTs is goodness even if any one particular client gets slightly more  
complex.

-Chris


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20071116/c187ae87/attachment.html>