[cfe-dev] Continuing Adventures with Objective-C

[Chris Lattner]

On May 10, 2008, at 10:49 AM, David Chisnall wrote:

> Hi Chris,
>
> Here's the latest diff - it's getting quite big, but you can ignore  
> most of the stuff in CGObjCEtoile.cpp (it's bitrotted a bit, since  
> I've changed some interfaces).

Hi David,

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.

Example pieces:
1) the changes for your runtime should be split out
2) the self/_cmd/super changes should be split out.
3) the @defs support should be split out
4) the ccc changes should be split out
5) the codegen refactoring (ConvertReturnType etc) should be split out.
6) the various random codegen improvements should be split out.


Questions:
What is the idea with ImplicitParamInfo in ObjCMethodDecl?  Should  
isObjCPointerType be a method in codegen, or something in the AST?

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



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


@@ -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.  Also please move the @ handling part to the "else" clause of  
the if, so that the normal struct case comes before @defs handling.


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.

-Chris



>
>
> There is lots of nasty stuff in there to do with casting at the  
> moment.  In Objective-C, you can implicitly cast object pointers to  
> other object pointers with some very lax type checking (any object  
> pointer can be mapped to or from id without an explict cast), which  
> doesn't really map well onto LLVM.   I've added special cases to all  
> of the bits of code where asserts were failing when building  
> GNUstep.  Some of these can be done cleanly - when we still have  
> QualTypes around we can check if they casting between ObjC pointer -  
> but in a couple I've had to just allow any pointer conversion (the  
> one that comes to mind is constructing Phi nodes after a shorthand- 
> if statement, where both sides return some kind of object) and hope  
> that Sema has already caught mismatched types.
>
> Casting a scalar to a union containing that scalar is partially  
> working, but not in all cases.  The remaining big issue with  
> compiling GNUstep is the lack of support for generating l-values as  
> a result of cast expressions.  I haven't tackled this at all because  
> I am not sure what the semantics of a taking the address of the  
> result of a cast are meant to be.
>
> I've tried to comment everything, but let me know if you have any of  
> it doesn't make sense.
>
> David
> <clang.diff>