[cfe-dev] Objective-C tidy up

Chris Lattner clattner at apple.com
Sat Jun 7 13:43:26 PDT 2008

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

>> Take a look at how name lookup is currently working and how builtins
>> are lazily created, you should be able to do something similar.
> This is fine for codegen.  Will other AST consumers object if  
> implicit parameter decls aren't there if they are not referenced?

Builtins are correct in the ast.  They are lazily generated by sema,  
but sema updates the AST and namelookup appropriately.

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

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

+++ 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>(

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.

+const llvm::Type *CodeGenTypes::ConvertReturnType(QualType T) {
+    if (T->isVoidType())
+      return llvm::Type::VoidTy;    // Result of function uses llvm  
+    else
+      return ConvertType(T);

Please indent by 2.

Otherwise the patch is looking great.  Thanks for tolerating me ;)


More information about the cfe-dev mailing list