[cfe-dev] Objective-C: methods, ivars, and cleanup

[Chris Lattner]

On Mar 24, 2008, at 11:48 AM, David Chisnall wrote:

> Hi,
>
> This is a fairly big diff, so I've tried to split it into a few  
> separate parts.

Great, thanks!

> globals.diff is a simple addition to the global_ctors code which  
> avoids emitting empty global_ctors.

A couple things here:

Instead of nesting the entire body of the function, please just use:

if (GlobalCtors.empty()) return;

Second, you're implicitly reverting a cleanup fix:

-  llvm::StructType* CtorStructTy =
-  llvm::StructType::get(llvm::Type::Int32Ty, FPType, NULL);

is better than:

+    std::vector<const llvm::Type*> CtorFields;
+    CtorFields.push_back(llvm::IntegerType::get(32));
+    // i32, function type pair
+    CtorFields.push_back(llvm::PointerType::getUnqual(CtorFuncTy));
+    llvm::StructType* CtorStructTy =  
llvm::StructType::get(CtorFields, false);

Likewise the fix to inline GlobalCtorsVar and several other things.


Finally, please make sure the code fits in 80 columns.


> etoile_runtime.diff contains a subclass of CGObjCRuntime which  
> implements the runtime-specific parts for the Étoilé runtime.  Other  
> parts do not depend on this unless output for this runtime is  
> selected (the runtime to use is currently hard-coded).  I am now  
> writing the GNU and Étoilé versions at the same time to make sure  
> the interfaces are sufficiently abstract.  Hopefully this will make  
> adding support for the Apple runtimes easy for whoever gets that job.

Ok. I'll let you worry about the correctness of this code :).  Please  
make sure you follow coding conventions though.  For example, don't  
include commented out code:

+/*
+clang::CodeGen::CGObjCRuntime *clang::CodeGen::CreateObjCRuntime(
+    llvm::Module &M,
+    const llvm::Type *LLVMIntType,
+    const llvm::Type *LLVMLongType) {
+  return new CGObjCEtoile(M, LLVMIntType, LLVMLongType);
+}
+*/

Please fit in 80 cols, please don't use macros:

+#define SET(structure, index, value) do {\
+    llvm::Value *element_ptr = Builder.CreateStructGEP(structure,  
index);\
+    Builder.CreateStore(value, element_ptr);} while(0)

(use a static function instead)


Stuff like this won't work on 64-bit platforms:

   llvm::Constant *SelFunction =
+    TheModule.getOrInsertFunction("lookup_typed_selector",
+        SelectorTy,
+        PtrToInt8Ty,
+        PtrToInt8Ty,
+        0);
+

Use NULL instead of 0.  Likewise for StructType::get and other  
variadic functions.

+#include <stdarg.h>

I don't think you need this, but if you do, please use <cstdarg>  
instead.

+//===------- CGObjCEtoile.cpp - Emit LLVM Code from ASTs for a Module  
--------===//

This should follow the formating of the other files more closely.


> obj_struct.diff handles conversion from Objective-C pointers to LLVM  
> types.  This is needed for accessing instance variables statically  
> and is used in the ivar access parts.  This patch is also self- 
> contained, and does not depend on any other changes.

In CodeGenTypes::CollectIvarTypes, please pass IvarTypes by reference  
instead of
by pointer: this is a small amount of documentation that the pointer  
can't be null.

Oh, I see Devang responded as well, I'll stop here :)

-Chris

>
>
> runtime_if.diff contains changes to the runtime interface.  This  
> probably needs to be committed at the same time as the changes to  
> the GNU runtime, or there might be some conflicts.  The Étoilé patch  
> may also conflict with the old definitions if it is committed before  
> this.
>
> gnu_runtime.diff contains the changes made to the GNU runtime to  
> adopt the new interface.  It also adds methods for generating method  
> functions and some things related to creating class structures which  
> are not used yet.
>
> method_gen.diff contains the code required to generate Objective-C  
> methods (although it does not yet attach them to class structures.
>
> Finally, ivar.diff contains the code required for accessing and  
> modifying Objective-C instance variables.  This depends on the stuff  
> in objc_struct.diff for all runtimes that do not use late-bound  
> instance variable addresses (i.e. all of the currently supported  
> ones).
>
> Have fun,
>
> David
>
> < 
> globals 
> .diff 
> > 
> < 
> gnu_runtime 
> .diff 
> > 
> < 
> ivar 
> .diff 
> > 
> < 
> method_gen 
> .diff 
> > 
> < 
> obj_struct 
> .diff 
> > 
> < 
> runtime_if 
> .diff 
> ><etoile_runtime.diff>_______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev