[cfe-commits] r59543 - in /cfe/trunk/lib/CodeGen: CGExpr.cpp CGObjCMac.cpp CGValue.h

[Fariborz Jahanian]

Ok. I wil do this along with the next patch.

- Fariborz

On Nov 18, 2008, at 3:59 PM, Daniel Dunbar wrote:

> Hi Fariborz,
>
> Some minor nitpicks:
>
> On Tue, Nov 18, 2008 at 12:18 PM, Fariborz Jahanian <fjahanian at apple.com 
> > wrote:
>>  } else if (VD && VD->isFileVarDecl()) {
>> -    return LValue::MakeAddr(CGM.GetAddrOfGlobalVar(VD),
>> -                            E->getType().getCVRQualifiers());
>> +    LValue LV = LValue::MakeAddr(CGM.GetAddrOfGlobalVar(VD),
>> +                                 E->getType().getCVRQualifiers());
>> +    if (VD->getAttr<ObjCGCAttr>())
>> +    {
>> +      ObjCGCAttr::GCAttrTypes attrType = (VD- 
>> >getAttr<ObjCGCAttr>())->getType();
>
> This is a little more idiomatic, also note clang tends to have braces
> on same line as condition:
>
> if (ObjCGCAttr *A = VD->getAttr<ObjCGCAttr>()) {
>     ObjCGCAttr::GCAttrTypes attrType = A->getType();
>
>> +  bool ObjcWeak:1;
>> +  bool ObjcStrong:1;
>
> Can this be changed to 'unsigned ObjCType : 2', with an appropriate
> enum for None, Weak, Strong? This makes it clear that we have a free
> value and that weak and strong are mutually exclusive. This simplifies
> the declaration of SetObjCGCAttrs (which I would call SetObjCType to
> match the field).
>
>> +  bool isObjcWeak() const { return ObjcWeak; }
>> +  bool isObjcStrong() const { return ObjcStrong; }
>
> These should be spelled ObjC for internal consistency.
>
> Thanks,
> - Daniel