[cfe-commits] r49565 - in /cfe/trunk: Driver/ASTConsumers.cpp include/clang/AST/DeclObjC.h include/clang/Parse/Action.h lib/AST/DeclObjC.cpp lib/Parse/ParseObjc.cpp lib/Sema/Sema.h lib/Sema/SemaDeclObjC.cpp

[Chris Lattner]

On Apr 11, 2008, at 4:40 PM, Fariborz Jahanian wrote:
> Author: fjahanian
>
> URL: http://llvm.org/viewvc/llvm-project?rev=49565&view=rev
> Log:
> AST generation for objc2's property declarations.

Nice Fariborz!

> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/include/clang/AST/DeclObjC.h (original)
> +++ cfe/trunk/include/clang/AST/DeclObjC.h Fri Apr 11 18:40:25 2008
> @@ -27,6 +27,7 @@
> class ObjCProtocolDecl;
> class ObjCCategoryDecl;
> class ObjCPropertyDecl;
> +class FieldDeclarator;

This shouldn't be needed.  FieldDeclarator is a parser level class,  
the AST classes shouldn't use it.

Also, DeclBase.h contains:

     // Decl
     //   NamedDecl
            Field,
            ...
            ObjCProtocol,
            ObjCProperty,

Since ObjCProperty isn't a named decl, it should be moved in the enum  
list.  You don't want isa<NamedDecl>(property) to return true.


class ObjCPropertyDecl : public Decl {
public:

Please add a bit explanatory comment above this, explaining what  
ObjCPropertyDecl's are.  The comment above ObjCImplementationDecl or  
ObjCCategoryDecl are great examples to follow.


>   propdecl_iterator propdecl_begin() const { return PropertyDecls; }
>   propdecl_iterator propdecl_end() const {
>     return PropertyDecls+NumPropertyDecls;
>   }
>   unsigned propdecl_size() const { return NumPropertyDecls; }
>   bool propdecl_empty() const { return NumPropertyDecls == 0; }
> +  NamedDecl **getPropertyDecls() { return PropertyDecls; }
> +  void setNumPropertyDecls(unsigned num) { NumPropertyDecls = num; }
> +  void setPropertyDecls(NamedDecl **Nd) { PropertyDecls = Nd; }

The rest of the objc decl classes use stuff like:

   void addMethods(ObjCMethodDecl **insMethods, unsigned numInsMembers,
                   ObjCMethodDecl **clsMethods, unsigned numClsMembers,
                   SourceLocation AtEndLoc);

Please be consistent: make one method take the list and the count, and  
name it "addXXX".

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/lib/Parse/ParseObjc.cpp (original)
> +++ cfe/trunk/lib/Parse/ParseObjc.cpp Fri Apr 11 18:40:25 2008
> @@ -392,26 +392,15 @@
>   DeclSpec DS;
>   llvm::SmallVector<FieldDeclarator, 8> FieldDeclarators;
>   ParseStructDeclaration(DS, FieldDeclarators);
> -
> -  // Convert them all to fields.
> -  for (unsigned i = 0, e = FieldDeclarators.size(); i != e; ++i) {
> -    FieldDeclarator &FD = FieldDeclarators[i];
> -    // Install the declarator into interfaceDecl.
> -    DeclTy *Field = Actions.ActOnIvar(CurScope,
> -                                        
> DS.getSourceRange().getBegin(),
> -                                      FD.D, FD.BitfieldSize,
> -                                      tok::objc_not_keyword);
> -    PropertyDecls.push_back(Field);
> -  }

Nice!

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/lib/Sema/SemaDeclObjC.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclObjC.cpp Fri Apr 11 18:40:25 2008
> @@ -886,43 +886,52 @@
> +Sema::DeclTy *Sema::ActOnAddObjCProperties(Scope *S, SourceLocation  
> AtLoc,
> +                                           FieldDeclarator  
> *propertyDeclarators,
> +                                           unsigned  
> NumPropertyDeclarators,
> +                                           ObjCDeclSpec &ODS) {
...

> +  if (NumPropertyDeclarators != 0) {
> +    NamedDecl **propertyName = new  
> NamedDecl*[NumPropertyDeclarators];
> +    PDecl->setPropertyDecls(propertyName);
> +    PDecl->setNumPropertyDecls(NumPropertyDeclarators);

Sema should not be doing the memory allocation here, the PropertyDecl  
class should be doing this in the "addProperties" method or whatever.


>
> +    for (unsigned i = 0; i < NumPropertyDeclarators; i++) {
> +      Declarator &D = propertyDeclarators[i].D;
> +      propertyName[i] = new NamedDecl(Decl::ObjCProperty,
> +                                      D.getIdentifierLoc(),  
> D.getIdentifier());

You should not 'new' NamedDecl, it is supposed to be an abstract  
class.  After this gets fixed, we should mark its ctor 'protected'.

Further, you set the 'kind' to ObjCProperty even though the decl is  
not an instance of ObjCPropertyDecl.

>
> +    }
> +  }

Finally, one *big* question: you seem to model:

@property int x, y, z;

as one PropertyDecl with three "NamedDecl"'s hanging off it.  Why not  
model this as three PropertyDecls?  This way PropertyDecl could really  
be a NamedDecl, and this more closely matches how we handle ivars and  
struct fields.

-Chris