[cfe-commits] r59682 - in /cfe/trunk: include/clang/Basic/DiagnosticKinds.def lib/CodeGen/CGExpr.cpp lib/CodeGen/CodeGenFunction.h lib/Sema/SemaDeclAttr.cpp

[Daniel Dunbar]

One more comment I forgot. Please add a test case for the diagnostic.
Our -verify option is quite useful for this, see the tests in
test/Sema for many examples of what to do.

 - Daniel

On Thu, Nov 20, 2008 at 9:04 AM, Daniel Dunbar <daniel at zuster.org> wrote:
> Hi Fariborz,
>
> A few comments:
>
> For readability consider splitting commits which hit several layers
> (Parser/Sema/CodeGen) into independent commits.
>
> On Wed, Nov 19, 2008 at 4:15 PM, Fariborz Jahanian <fjahanian at apple.com> wrote:
>> +DIAG(warn_attribute_weak_on_field, WARNING,
>> +     "__weak attribute cannot be specified on a field declaration")
>
> This spelling strikes me as odd, since __weak is just a define. I
> presume this matches gcc though?
>
>> +/// SetVarDeclObjCAttribute - Set __weak/__strong attributes into the LValue
>> +/// object.
>> +void CodeGenFunction::SetVarDeclObjCAttribute(const VarDecl *VD,
>> +                                              const QualType &Ty,
>> +                                              LValue &LV)
>> +{
>
> I think this should be a static function instead of a member function,
> which is slightly preferable. It just needs the ASTContext as an
> argument (the ASTContext has a getLangOptions).
>
>> +  if (const ObjCGCAttr *A = VD->getAttr<ObjCGCAttr>()) {
>> +    ObjCGCAttr::GCAttrTypes attrType = A->getType();
>> +    LValue::SetObjCType(attrType == ObjCGCAttr::Weak,
>> +                        attrType == ObjCGCAttr::Strong, LV);
>> +  }
>> +  else if (CGM.getLangOptions().ObjC1 &&
>> +           CGM.getLangOptions().getGCMode() != LangOptions::NonGC) {
>> +    if (getContext().isObjCObjectPointerType(Ty))
>> +      LValue::SetObjCType(false, true, LV);
>
> Although its fairly obvious what this is doing for ObjC-1, can you add
> a comment about it, preferably with a reference to something (e.g., a
> radar) which defines this behavior if one exists.
>
>> +  }
>> +}
>>
>>  LValue CodeGenFunction::EmitDeclRefLValue(const DeclRefExpr *E) {
>>   const VarDecl *VD = dyn_cast<VarDecl>(E->getDecl());
>>
>>   if (VD && (VD->isBlockVarDecl() || isa<ParmVarDecl>(VD) ||
>>         isa<ImplicitParamDecl>(VD))) {
>> -    if (VD->getStorageClass() == VarDecl::Extern)
>> -      return LValue::MakeAddr(CGM.GetAddrOfGlobalVar(VD),
>> -                              E->getType().getCVRQualifiers());
>> +    LValue LV;
>> +    if (VD->getStorageClass() == VarDecl::Extern) {
>> +      LV = LValue::MakeAddr(CGM.GetAddrOfGlobalVar(VD),
>> +                            E->getType().getCVRQualifiers());
>> +    }
>>     else {
>>       llvm::Value *V = LocalDeclMap[VD];
>>       assert(V && "BlockVarDecl not entered in LocalDeclMap?");
>> -      return LValue::MakeAddr(V, E->getType().getCVRQualifiers());
>> +      LV = LValue::MakeAddr(V, E->getType().getCVRQualifiers());
>>     }
>> +    if (VD->isBlockVarDecl() &&
>> +        (VD->getStorageClass() == VarDecl::Static ||
>> +         VD->getStorageClass() == VarDecl::Extern))
>> +      SetVarDeclObjCAttribute(VD, E->getType(), LV);
>> +    return LV;
>>   } else if (VD && VD->isFileVarDecl()) {
>>     LValue LV = LValue::MakeAddr(CGM.GetAddrOfGlobalVar(VD),
>>                                  E->getType().getCVRQualifiers());
>> -    if (const ObjCGCAttr *A = VD->getAttr<ObjCGCAttr>()) {
>> -      ObjCGCAttr::GCAttrTypes attrType = A->getType();
>> -      LValue::SetObjCType(attrType == ObjCGCAttr::Weak, attrType == ObjCGCAttr::Strong, LV);
>> -    }
>> -    else if (CGM.getLangOptions().ObjC1 &&
>> -             CGM.getLangOptions().getGCMode() != LangOptions::NonGC) {
>> -      QualType ExprTy = E->getType();
>> -      if (getContext().isObjCObjectPointerType(ExprTy))
>> -        LValue::SetObjCType(false, true, LV);
>> -    }
>> +    SetVarDeclObjCAttribute(VD, E->getType(), LV);
>>     return LV;
>>   } else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(E->getDecl())) {
>>     return LValue::MakeAddr(CGM.GetAddrOfFunction(FD),
>> @@ -767,7 +782,7 @@
>>                                 Field->getType()->isSignedIntegerType(),
>>                             Field->getType().getCVRQualifiers()|CVRQualifiers);
>>   }
>> -
>> +
>>   V = Builder.CreateStructGEP(BaseValue, idx, "tmp");
>>
>>   // Match union field type.
>> @@ -782,8 +797,21 @@
>>                               "tmp");
>>   }
>>
>> -  return LValue::MakeAddr(V,
>> -                          Field->getType().getCVRQualifiers()|CVRQualifiers);
>> +  LValue LV =
>> +    LValue::MakeAddr(V,
>> +                     Field->getType().getCVRQualifiers()|CVRQualifiers);
>> +  if (const ObjCGCAttr *A = Field->getAttr<ObjCGCAttr>()) {
>> +    ObjCGCAttr::GCAttrTypes attrType = A->getType();
>> +    // __weak attribute on a field is ignored.
>> +    LValue::SetObjCType(false, attrType == ObjCGCAttr::Strong, LV);
>
> This special casing for weak shouldn't need to be done here, instead
> Sema should make sure this situation does not occur. I think this can
> simply assert that this holds and then use SetObjCType.
>
>> +  }
>> +  else if (CGM.getLangOptions().ObjC1 &&
>> +           CGM.getLangOptions().getGCMode() != LangOptions::NonGC) {
>> +    QualType ExprTy = Field->getType();
>> +    if (getContext().isObjCObjectPointerType(ExprTy))
>> +      LValue::SetObjCType(false, true, LV);
>> +  }
>> +  return LV;
>>  }
>>
>>  LValue CodeGenFunction::EmitCompoundLiteralLValue(const CompoundLiteralExpr* E)
>>
>> Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.h?rev=59682&r1=59681&r2=59682&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/CodeGenFunction.h (original)
>> +++ cfe/trunk/lib/CodeGen/CodeGenFunction.h Wed Nov 19 18:15:42 2008
>> @@ -462,7 +462,8 @@
>>   LValue EmitBinaryOperatorLValue(const BinaryOperator *E);
>>   // Note: only availabe for agg return types
>>   LValue EmitCallExprLValue(const CallExpr *E);
>> -
>> +  void SetVarDeclObjCAttribute(const VarDecl *VD, const QualType &Ty,
>> +                               LValue &LV);
>>   LValue EmitDeclRefLValue(const DeclRefExpr *E);
>>   LValue EmitStringLiteralLValue(const StringLiteral *E);
>>   LValue EmitPredefinedFunctionName(unsigned Type);
>>
>> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=59682&r1=59681&r2=59682&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Wed Nov 19 18:15:42 2008
>> @@ -555,8 +555,11 @@
>>
>>
>>   ObjCGCAttr::GCAttrTypes type;
>> -  if (Attr.getParameterName()->isName("weak"))
>> +  if (Attr.getParameterName()->isName("weak")) {
>> +    if (isa<FieldDecl>(d))
>> +      S.Diag(Attr.getLoc(), diag::warn_attribute_weak_on_field);
>>     type = ObjCGCAttr::Weak;
>
> This shouldn't add the weak attribute on the path with the diagnostic.
>
> Thanks,
>  - Daniel
>