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

[Fariborz Jahanian]

On Nov 16, 2007, at 7:05 AM, Steve Naroff wrote:

>
> On Nov 15, 2007, at 10:21 AM, Chris Lattner wrote:
>
>> On Nov 15, 2007, at 9:33 AM, Fariborz Jahanian wrote:
>>>>> --- cfe/trunk/include/clang/AST/Expr.h (original)
>>>>> +++ cfe/trunk/include/clang/AST/Expr.h Wed Nov 14 20:58:25 2007
>>>>> @@ -1256,18 +1256,23 @@
>>>>> class ObjcIvarDecl *D;
>>>>> SourceLocation Loc;
>>>>> Expr *Base;
>>>>> -  bool IsArrow;      // True if this is "X->F", false if this is
>>>>> "X.F".
>>>>> +  bool IsArrow:1;      // True if this is "X->F", false if this  
>>>>> is
>>>>> "X.F".
>>>>> +  bool IsFreeIvar:1;   // True if ivar reference has no base  
>>>>> (self
>>>>> assumed).
>>>>
>>>> What is IsFreeIvar used for?  Shouldn't references to fields of the
>>>> current objects just use a declrefexpr?
>>>
>>> It is ivar reference without the base expressions ('self') is  
>>> assumed. This is to 'attach' the base during rewrite and also for  
>>> pretty printing to come out exactly as user wrote the code. Use of  
>>> declrefexpr probably masks what user 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;
> }
>
> 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).
>
> 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);
>       }
>

This is actually how I started out with. But I ran into two issues; 1)  
I did not find it easy to attach the base 'self' during rewrite.  
'rewrite' is not in the business of name look up. 2) AST IMO, should  
completely represent the semantics of the language. Leaving out 'self'  
and assuming its 'clients' know what to do violates this assumption,  
IMO.

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

This addresses the issue 2) issue above. But I still don't know if it  
is job of clients to find/build a 'self'/'this' expression.

- Fariborz

>
> snaroff
>

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