[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

[Fariborz Jahanian]

On Apr 13, 2008, at 3:16 PM, Chris Lattner wrote:

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

And it is unnecessary and unused. Removed.

>
>
> 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.

This is leftover from the older patch. Will be taken care of.

>
>
>
> 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".

Sure.

>
>
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- 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.
OK.

>
>
>
>>
>> +    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'.

This is news to me. See below.

>
> 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.

My goal was to preserve the syntax of a property declaration in AST.  
So, we can rewrite it exactly as is written. I ASTed property for what  
they are; list of attributes, a type, and list of names.  But now that  
you foresee a NamedDecl to be an abstract class in the future, I will  
change them as you suggested. Assuming that we don;t care about  
preserving the syntactic information exactly.

- fariborz

>
>
> -Chris
>
>