Hi Chris,
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
cast.diff (4.34 KB)
ccc.diff (676 Bytes)
const_str.diff (1.76 KB)
defs.diff (5.51 KB)
etoile.diff (5.96 KB)
expr.diff (9.5 KB)
function.diff (6.28 KB)
gnu.diff (38.9 KB)
implicit.diff (4.13 KB)
module.diff (10.2 KB)
objc.diff (8.5 KB)
override.diff (4.99 KB)
runtimeif.diff (5.44 KB)
static.diff (616 Bytes)
super.diff (1.9 KB)
types.diff (3.13 KB)
union.diff (1.61 KB)
vla.diff (587 Bytes)
aggmesg.diff (1.81 KB)
