[cfe-dev] Continuing Adventures with Objective-C

Tue May 13 09:22:36 PDT 2008

Hi Chris,

On 12 May 2008, at 20:05, Chris Lattner wrote:

> This is huge.  I can't review this.  I see a whole bunch of  
> unrelated changes, please split this out into one patch per change.   
> This is important for review, but it is also important for revision  
> control purposes (you can revert one patch without all the others).   
> Also, this allows patch review to be done by different people which  
> might be specialized in certain areas.

Sorry about the size.  It's a bit hard to split it up, since many of  
the changes depend on others.  I'm now using svk for clang so I can  
split future diffs up more easily once my tree is a bit better sync'd  
with trunk.

> Example pieces:
> 1) the changes for your runtime should be split out

The runtime-specific code is all in GCObjCGNU.cpp and  
CGObjCEtoile.cpp.  It's hard to split these out as separate diffs  
because these changes depend on changes to the interface declared in  
CGObjCRuntime.h, which in turn depend on changes in other bits of the  
code that call it.

The Étoilé runtime stuff is still in a state of flux, so I have  
enclosed a simplified version of the diff that only changes the  
interfaces to match the new versions. (etoile.diff)

The GNU runtime specific stuff is all in gnu.diff.

The new runtime interface is in runtimeif.diff.  This can not be  
committed separately from the changes to the runtime implementations  
and the things in CGObjC.cpp that call it.

> 2) the self/_cmd/super changes should be split out.

Super is in super.diff.  Implicit parameters, which include self and  
_cmd are in implicit.diff.

> 3) the @defs support should be split out

This is in defs.diff.

> 4) the ccc changes should be split out

ccc.diff (These are unchanged from the ccc.diff in the earlier email).

> 5) the codegen refactoring (ConvertReturnType etc) should be split  
> out.

types.diff contains the changes to CodeGenTypes which allow converting  
a return type.  This is required for the changes to  
CodeGenFunction::GenerateObjCMethod().

function.diff pulls shared code from CodeGenFunction::GenrateCode and  
GenerateObjCMethod out.  It also removes the method for generating  
ObjC methods from CodeGenFunction.

objc.diff puts this method and other ObjC specific parts of  
CodeGenFunction into GCObjC.cpp (where I should have put them to start  
with).

module.diff contains the runtime-agnostic code for generating module- 
level ObjC constructs (classes, categories and protocols).  This  
depends on the new runtime interface since the old one did not have  
methods for doing any of this.

static.diff fixes static variables in ObjC methods.

override.diff allows the types of id and Class to be redefined.  This  
happens whenever a runtime-specific header file is included.  I think  
GCC avoids this problem by including these headers itself and picking  
up the declarations from there.

union.diff adds support for GCC's cast-to-union extension, which is  
used in a depressing number of places in the GNUstep code.

cast.diff fixes a number of cases where implicit casts are not  
correctly codegen'd, and allows Objective-C const id to have messages  
sent to it.

expr.diff contains a load of small tidies (i.e. everything I couldn't  
thing of which diff to put it in)

> 6) the various random codegen improvements should be split out.

const_str.diff adds a call to the runtime-specific method for  
generating constant ObjC strings.  It also relaxes the string class  
type checking.  If NSConstantString has not been declared when a  
constant string is encountered (which happens quite often) then the  
constant string is an id.  If it has, then it is an NSConstantString.

aggmsg.diff generates message sends that return aggregate types by  
calling runtime-specific methods.  There are corresponding changes in  
the runtime-specific code to make this actually work.

> Questions:
> What is the idea with ImplicitParamInfo in ObjCMethodDecl?

It allows enumeration of things like self and _cmd.  For the Étoilé  
runtime it will also provide support for _call (which is mainly used  
by prototype-base languages such as Io and JavaScript, but might be  
useful to export to Objective-C programmers).  In a future patch I  
will factor the code that sets these out into a runtime-specific  
class.  These are just function parameters, so they do not need any  
special handling (the existing code for accessing local variables  
works on them in the codegen).

This is basically a generalisation of the existing selfDecl stuff.  At  
the moment, self and _cmd are hard-coded.  When this is committed I  
will allow other implicit parameters to be defined per-runtime.

> Should isObjCPointerType be a method in codegen, or something in the  
> AST?

It is in CodeGen because it is only used in CodeGen.  It is used to  
decide when two pointers which point to different LLVM types can be  
implicitly bitcast - AST is unaware of LLVM types so it doesn't make  
this distinction.  This is probably not the ideal solution, so  
suggestion are welcome.

> +++ lib/AST/StmtPrinter.cpp	(working copy)
> @@ -496,6 +496,9 @@
>     case PreDefinedExpr::PrettyFunction:
>       OS << "__PRETTY_FUNCTION__";
>       break;
> +    case PreDefinedExpr::ObjCSuper:
> +      OS << "super";
> +      break;
>
> this should handle self/_cmd also.

self and _cmd are handled by the implicit parameter code now.  They  
work as any other variable references, both when accessing them in  
codegen and when doing an AST print.  super is different because it is  
an alias for self with special semantics.  These should not have been  
left in Expr.h diff, since they are never used.

>   // typedef struct objc_class *Class;
>   const PointerType *ptr = TD->getUnderlyingType()- 
> >getAsPointerType();
> -  assert(ptr && "'Class' incorrectly typed");
> +  //assert(ptr && "'Class' incorrectly typed");
>   const RecordType *rec = ptr->getPointeeType()->getAsStructureType();
> -  assert(rec && "'Class' incorrectly typed");
> +  //assert(rec && "'Class' incorrectly typed");
>   ClassStructType = rec;
>
> Why are you commenting out code?

Because it was breaking something else which I have now fixed and I  
forgot to uncomment them.  The asserts requiring SEL to be a pointer  
type will have to be changed at some point since SEL is an i32 in the  
Étoilé runtime.  (SEL is an opaque type with respect to the language.   
The fact that it is a pointer in the GNU and NeXT runtimes is an  
implementation detail and doesn't belong in the AST).

> @@ -721,16 +721,32 @@
>     // Parse all the comma separated declarators.
>     DeclSpec DS;
>     FieldDeclarators.clear();
> +    if(Tok.is(tok::at)) {
> +      ConsumeToken();
> +      //FIXME: Turn these into helpful errors
> +      assert(Tok.isObjCAtKeyword(tok::objc_defs) && "defs expected");
> +      ConsumeToken();
> +      assert(Tok.is(tok::l_paren) && "( expected");
>
> Please do the fixme: checking in code that is known broken is badness.

Sorry, I hadn't read the diagnostics code when I wrote this.  Fixed  
now.  I've also replaced the corresponding assert in SemaDecl with an  
error.

> Also please move the @ handling part to the "else" clause of the if,  
> so that the normal struct case comes before @defs handling.

Done.

> This is great work and I'm thrilled that you're making such huge  
> enhancements to clang, but I would also really like to get it  
> checked into clang... and we can't do that until it is split up a  
> bit more.

I've stopped working on new things to focus on getting the current  
changes merged so I don't diverge any further.  Let me know what needs  
changing to get it committed.

David

-------------- next part --------------
A non-text attachment was scrubbed...
Name: cast.diff
Type: application/octet-stream
Size: 4440 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ccc.diff
Type: application/octet-stream
Size: 676 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: const_str.diff
Type: application/octet-stream
Size: 1807 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: defs.diff
Type: application/octet-stream
Size: 5643 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0003.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: etoile.diff
Type: application/octet-stream
Size: 6101 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0004.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: expr.diff
Type: application/octet-stream
Size: 9726 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0005.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: function.diff
Type: application/octet-stream
Size: 6428 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0006.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gnu.diff
Type: application/octet-stream
Size: 39856 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0007.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: implicit.diff
Type: application/octet-stream
Size: 4231 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0008.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: module.diff
Type: application/octet-stream
Size: 10431 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0009.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: objc.diff
Type: application/octet-stream
Size: 8706 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0010.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: override.diff
Type: application/octet-stream
Size: 5106 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0011.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: runtimeif.diff
Type: application/octet-stream
Size: 5574 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0012.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: static.diff
Type: application/octet-stream
Size: 616 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0013.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: super.diff
Type: application/octet-stream
Size: 1948 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0014.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: types.diff
Type: application/octet-stream
Size: 3207 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0015.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: union.diff
Type: application/octet-stream
Size: 1652 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0016.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vla.diff
Type: application/octet-stream
Size: 587 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0017.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: aggmesg.diff
Type: application/octet-stream
Size: 1856 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080513/d6020a91/attachment-0018.obj>