[cfe-commits] r49506 - in /cfe/trunk: include/clang/Parse/Action.h lib/Parse/ParseDecl.cpp lib/Parse/ParseObjc.cpp lib/Sema/Sema.h lib/Sema/SemaDecl.cpp

[Chris Lattner]

On Apr 10, 2008, at 4:32 PM, Fariborz Jahanian wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=49506&view=rev
> Log:
> Patch for:
> 1) objc ivar processing is split out of ActOnField into its own  
> ActOnIvar method.
> 2) the new objc ivar action takes visibility info directly,  
> eliminating
>  AllVisibilities in ParseObjCClassInstanceVariables.

Nice!

> --- cfe/trunk/include/clang/Parse/Action.h (original)

>
> +  virtual DeclTy *ActOnIvar(Scope *S, SourceLocation DeclStart,
> +                            Declarator &D, ExprTy *BitfieldWidth,
> +                            tok::ObjCKeywordKind visibility =  
> tok::objc_not_keyword) {

I don't think that visibility needs to be defaulted, the only client  
of this method always specifies it.

> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Thu Apr 10 18:32:45 2008
> @@ -1460,19 +1459,7 @@
>   // FIXME: Chain fielddecls together.
>   FieldDecl *NewFD;
>
> -  if (isa<RecordDecl>(TagDecl))
> -    NewFD = FieldDecl::Create(Context, Loc, II, T, BitWidth);
> -  else if (isa<ObjCInterfaceDecl>(TagDecl) ||
> -           isa<ObjCImplementationDecl>(TagDecl) ||
> -           isa<ObjCCategoryDecl>(TagDecl) ||
> -           // FIXME: ivars are currently used to model properties,  
> and
> -           // properties can appear within a protocol.
> -           // See corresponding FIXME in DeclObjC.h:ObjCPropertyDecl.
> -           isa<ObjCProtocolDecl>(TagDecl))
> -    NewFD = ObjCIvarDecl::Create(Context, Loc, II, T);
> -  else
> -    assert(0 && "Sema::ActOnField(): Unknown TagDecl");
> -

Nice!

>
> +  NewFD = FieldDecl::Create(Context, Loc, II, T, BitWidth);

Please merge this into the declaration of NewFD.

> +/// ActOnIvar - Each field of a struct/union/class is passed into  
> this in order
> +/// to create an IvarDecl object for it.

Please update this comment, this doesn't apply to s/u/c's.

>
> +Sema::DeclTy *Sema::ActOnIvar(Scope *S,
> +                               SourceLocation DeclStart,
> +                               Declarator &D, ExprTy *BitfieldWidth,
> +                              tok::ObjCKeywordKind visibility) {

Please capitalize 'visibility' for consistency with the rest of the  
code.  Also, the middle two lines should be indented one space less.

>
> +  IdentifierInfo *II = D.getIdentifier();
> +  Expr *BitWidth = (Expr*)BitfieldWidth;
> +  SourceLocation Loc = DeclStart;
> +  if (II) Loc = D.getIdentifierLoc();

Can ivars be bitfields?  If not, they should just be rejected.

> +
> +  // FIXME: Unnamed fields can be handled in various different  
> ways, for
> +  // example, unnamed unions inject all members into the struct  
> namespace!

This needs to be updated.  Can ivars be unnamed?

> +  if (BitWidth) {
> +    // TODO: Validate.
> +    //printf("WARNING: BITFIELDS IGNORED!\n");
> +
> +    // 6.7.2.1p3
> +    // 6.7.2.1p4
> +
> +  } else {
> +    // Not a bitfield.
> +
> +    // validate II.
> +
> +  }

This should probably go away.

> +
> +  QualType T = GetTypeForDeclarator(D, S);
> +  assert(!T.isNull() && "GetTypeForDeclarator() returned null type");
> +  bool InvalidDecl = false;
> +
> +  // C99 6.7.2.1p8: A member of a structure or union may have any  
> type other
> +  // than a variably modified type.
> +  if (T->isVariablyModifiedType()) {
> +    // FIXME: This diagnostic needs work
> +    Diag(Loc, diag::err_typecheck_illegal_vla, Loc);
> +    InvalidDecl = true;
> +  }
> +

> +  ObjCIvarDecl *NewID;
> +
> +  NewID = ObjCIvarDecl::Create(Context, Loc, II, T);

please merge these.

> +  HandleDeclAttributes(NewID, D.getDeclSpec().getAttributes(),
> +                       D.getAttributes());
> +
> +  if (D.getInvalidType() || InvalidDecl)
> +    NewID->setInvalidDecl();
> +  // If we have visibility info, make sure the AST is set  
> accordingly.
> +  if (visibility != tok::objc_not_keyword)
> +    NewID ->setAccessControl(TranslateIvarVisibility(visibility));

No space before the ->.

Thanks Fariborz,

-Chris