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

[Steve Naroff]

On Nov 16, 2007, at 9:10 AM, Chris Lattner wrote:

>>> 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);
>

I agree (and came to the same conclusion over my workout this morning).

Decision: We replace ObjCIvarRefExpr with MemberExpr and DeclRefExpr.

> 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.
>

I totally agree...the AST's need to be "right", independent of any  
particular application.

That said, while this is simplifying, it does increase the complexity  
of determining if an expression is actually an ivar reference.

No biggy, but worth mentioning (since complexity can be viewed from  
different perspectives...).

snaroff

> -Chris
>
>

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