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

[Steve Naroff]

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

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.

snaroff

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