[cfe-dev] Objective-C tidy up

[David Chisnall]

On 7 Jun 2008, at 21:43, Chris Lattner wrote:

>>>>>> Do we need a new subclass?  I'm not actually sure what this needs
>>>> that a ValueDecl doesn't provide, so we could possibly just use a
>>>> ValueDecl.
>>>
>>> If that works, please go for it.  Please add a big comment above te
>>> ValueDecl class indicating that ObjC uses it this way though.
>>
>> ValueDecl's constructor is currently private, so I've added a public
>> Create() method.  This seems to work.  I wasn't using anything
>> specific to the param decl, so (after fixing a load of compiler
>> complaints) it is working again.
>
> Ok, this looks good.  You should probably use a specific Decl enum
> value for them though.  Would it be useful to add a subclass of
> ValueDecl just to support easy use of :
>
> if (isa<FooDecl>(X))
>
> That would check for the enum?  In fact, I really do think this would
> be preferable, just because the name ValueDecl really does seem like
> an abstract type.  What do you think?

I've created ImplicitParamDecl as a subclass of ValueDecl (which  
declares no new state or methods) and moved the static constructor  
into this class.

>>>> ry rare to have a class which does not contain at least one
>>>> reference to self.  Most classes will override -init, and the  
>>>> common
>>>> pattern for this is to start the method with:
>>>>
>>>> if (nil == (self = [self init])) return nil;
>>>
>>> Sure, but we'd need to make one decl for self in *every method* that
>>> references it, not once per class.
>>
>> Can we not have one decl object instance per class, but one decl map
>> entry (pointing to this instance)?
>
> As we discussed on irc, this is a totally awesome idea.  Lets talk
> about this as a followup patch though.

Sure - let me get the stuff I've already done tidied and committed  
before I start on new stuff...

>>>> [1] These should probably have a common superclass, since they are
>>>> almost identical - in fact an ObjCImplementationDecl could inherit
>>>> from ObjCCategoryImplDecl, since a class is effectively a category
>>>> which is allowed to declare instance variables.
>>>
>>> I think it would be great to unify some of this stuff.  There is a
>>> lot
>>> of awkward code in the objc front-end for groking this stuff that
>>> should be unified, but I haven't figured out how.  Suggestions
>>> welcome :)
>>
>> I still have about 2000 lines of uncommitted diffs, but when my tree
>> has sync'd up with trunk a bit better then I'll take a look at the
>> front end a bit and see what can be unified.  There are a few other
>> strange things there, such as the 'isArrow' property on ivar refs
>> which, unless I am missing something important, can never be false.
>
> Ok!  We welcome cleanup for the ObjC ASTs!
>
> Some other thoughts about your patch:
>
> +++ include/clang/AST/DeclObjC.h	(working copy)
> @@ -91,7 +91,9 @@
>    // The following are only used for method definitions, null
> otherwise.
>    // FIXME: space savings opportunity, consider a sub-class.
>    Stmt *Body;
> -  ParmVarDecl *SelfDecl;
> +  // Decls for implicit parameters
> +  ValueDecl *ImplicitParamInfo[4];
> +  unsigned NumImplicitParams;
>
>
> Please turn this into SelfDecl/CmdDecl/etc with explicit getters/
> setters instead of providing iteration with a count.

Done.

> +++ lib/CodeGen/CodeGenFunction.cpp	(working copy)
> @@ -27,7 +27,11 @@
>
>  CodeGenFunction::CodeGenFunction(CodeGenModule &cgm)
>    : CGM(cgm), Target(CGM.getContext().Target), SwitchInsn(NULL),
> -    CaseRangeBlock(NULL) {}
> +    CaseRangeBlock(NULL) {
> +    LLVMIntTy = ConvertType(getContext().IntTy);
> +    LLVMPointerWidth = static_cast<unsigned>(
> +
> getContext
> ().getTypeSize(getContext().getPointerType(getContext().VoidTy)));
> +}
>
> Watch out for 80 columns.  You can directly get the size of a pointer
> from TargetData instead of cons'ing up a pointer type and asking for
> its size.

I moved this code here from GenerateCode() (someone else wrote it).   
I've now modified it to get the number from Target - please check it  
is actually doing the right thing...

> +const llvm::Type *CodeGenTypes::ConvertReturnType(QualType T) {
> +    if (T->isVoidType())
> +      return llvm::Type::VoidTy;    // Result of function uses llvm
> void.
> +    else
> +      return ConvertType(T);
> +}
>
> Please indent by 2.

Fixed.

David