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

[Chris Lattner]

>>> +  assert(!verifyFunction(*CurFn));
>>
>> nit-pick, add a message here.
>
> That code was copied-and-pasted from the GenerateCode method used  
> for generating functions.  For completeness I've added a message in  
> to where I originally copied it from as well.

Hi David,

Some more minor nit-picky stuff (this is why small patches are better  
than big ones ;-):

+++ lib/CodeGen/CGObjCGNU.cpp	(working copy)

+#include <stdarg.h>

Use <cstdarg> and move it to the end of the #include list if it is  
needed.

+  llvm::Value *GenerateIvarList(llvm::LLVMFoldingBuilder &Builder,
+      std::vector<llvm::Constant*> MethodNames,
+      std::vector<llvm::Constant*> MethodTypes,
+      std::vector<llvm::Constant*> MethodIMPs);

You're passing these vectors by value, I'd suggest passing them by  
(const) reference unless you really really want a copy made.  likewise  
in a few other places.

+  // Get the selector Type.
+  std::vector<const llvm::Type*> Str2(2, PtrToInt8Ty);
+  const llvm::Type *SelStructTy = llvm::StructType::get(Str2);

Please use the variadic version of STructType::get to avoid the vector.


+  // C string type.  Used in lots of places.
+  PtrToInt8Ty =
+    llvm::PointerType::getUnqual(llvm::Type::Int8Ty);
+  // Get the selector Type.
+  std::vector<const llvm::Type*> Str2(2, PtrToInt8Ty);
+  const llvm::Type *SelStructTy = llvm::StructType::get(Str2);
+  SelectorTy = llvm::PointerType::getUnqual(SelStructTy);
+  PtrToIntTy = llvm::PointerType::getUnqual(IntTy);
+  PtrTy = llvm::PointerType::getUnqual(llvm::Type::Int8Ty);

You call PointerType::getUnqual(llvm::Type::Int8Ty) twice here, please  
eliminate one.


+  // Object type
+  llvm::OpaqueType *OpaqueObjTy = llvm::OpaqueType::get();
+  llvm::Type *OpaqueIdTy = llvm::PointerType::getUnqual(OpaqueObjTy);
+  IdTy =  
llvm::PointerType::getUnqual(llvm::StructType::get(OpaqueIdTy, NULL));
+  OpaqueObjTy->refineAbstractTypeTo(IdTy);

This code is unsafe, you need to use a PATypeHolder as described here:
http://llvm.org/docs/ProgrammersManual.html#TypeResolve

Likewise in GenerateMethodList and other places you use  
refineAbstractTypeTo


+static void SetField(llvm::LLVMFoldingBuilder &Builder,
+                     llvm::Value *Structure,
+                     unsigned Index,
+                     llvm::Value *Value) {
+    llvm::Value *element_ptr = Builder.CreateStructGEP(Structure,  
Index);
+    Builder.CreateStore(Value, element_ptr);


Nice, much better than a macro ;-) ;-).  Please only indent the body  
by 2, not 4 though.  Also, you could just use:

+    Builder.CreateStore(Value, Builder.CreateStructGEP(Structure,  
Index));

at which point it might make more sense to just inline this everywhere  
it is used.


+  if(SelTypes == 0) {
+    llvm::Constant *SelFunction =
+      TheModule.getOrInsertFunction("sel_get_uid", SelectorTy,  
PtrToInt8Ty, NULL);
+    cmd = Builder.CreateCall(SelFunction, SelName);

80 cols.

+    llvm::SmallVector<llvm::Value*, 2> Args;
+    Args.push_back(SelName);
+    Args.push_back(SelTypes);
+    cmd = Builder.CreateCall(SelFunction, Args.begin(), Args.end());

There is no need to use a SmallVector when the size is fixed, I'd just  
use:

    llvm::Value *Args[] = { SelName, SelTypes };
    cmd = Builder.CreateCall(SelFunction, Args, Args+2);


+  for(unsigned int i=0 ; i<1 ; i++) {

urr?  A single iteration loop?



+  std::vector<const llvm::Type*> Args;
+  Args.push_back(SelfTy);
+  Args.push_back(SelectorTy);
+  for (unsigned i=0; i<ArgC ; i++) {
+    Args.push_back(ArgTy[i]);
+  }

Instead of the loop, you can use:

+  Args.insert(Args.end(), ArgTy, ArgTy+ArgC);


+  llvm::FunctionType *MethodTy = llvm::FunctionType::get(ReturnTy,  
Args, isVarArg);

80 cols



+  FnRetTy = OMD->getResultType(); ///.getCanonicalType();

Please remove this comment or improve it.


+  FnRetTy = CurFuncDecl->getType().getCanonicalType();
+  FnRetTy = cast<FunctionType>(FnRetTy)->getResultType();


This would be more idiomatic to write as:

FnRetTy = CurFuncDecl->getType()->getAsFunctionType()->getResultType();




+Value *ScalarExprEmitter::VisitObjCIvarRefExpr(ObjCIvarRefExpr *E) {

A lot of this code will be independent of whether the IVar is a  
scalar, complex or aggregate.  I'd suggest moving the "get the address  
of an ivar" piece into CGObjC*.cpp and just have VisitObjCIvarRefExpr  
be a simple function which calls into the "form address" part and then  
just does a load.  This can be done as a follow on patch, but will be  
required to get Complex and aggregate (e.g. struct) ivars  
working.  ... actually, isn't this function just  
"EmitObjCIvarRefLValue" ?



@@ -341,6 +345,17 @@
        return LValue::MakeAddr(CGM.GetAddrOfGlobalVar(VD, false));
      else {
        llvm::Value *V = LocalDeclMap[D];
+      // Check if it's an implicit argument
+      // TODO: Other runtimes may set non-argument hidden variables
+      if (!V) {
+        std::string VarName(D->getName());
+        llvm::Function::arg_iterator AI = CurFn->arg_begin();
+        do {
+          if(VarName == AI->getName()) {
+            return LValue::MakeAddr(AI);
+          }
+        } while(++AI);
+      }
        assert(V && "BlockVarDecl not entered in LocalDeclMap?");

This is incredibly inefficient, what are you trying to accomplish  
here?  I'd suggest removing this and handling it as part of a follow- 
on patch.


-Chris