[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]

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