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

[David Chisnall]

On 26 Mar 2008, at 18:29, Chris Lattner wrote:

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

Ooops.  Left-over from a thing that isn't used anymore.  Deleted.

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

I'm not using them at all yet, so feel free not to commit them yet...  
(Fixed, in the meantime)

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

Fixed.

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

Fixed.

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

I think I've got all of them - let me know if I missed any.  (Refining  
abstract types is now my answer to your earlier question of what can  
be done cleanly with a macro but not with a static function).

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

Fixed.

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

Probably true.  I'll consider making this change in the next revision.

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

Fixed.

> +    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);

Fixed.

> +  for(unsigned int i=0 ; i<1 ; i++) {
>
> urr?  A single iteration loop?

Ooops.  That was a placeholder.  That method and its companion are  
untested.  My next task is finishing class generation, at which point  
I can test them properly (and, hopefully, I will then be able to  
compile and link simple Objective-C programs and start writing some  
proper tests rather than just looking at the IR).

> +  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);

Thanks.  I'm still trying to remember how STL works...

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

Fixed.  And in a few other places I missed earlier.

> +  FnRetTy = OMD->getResultType(); ///.getCanonicalType();
>
> Please remove this comment or improve it.

Ooops, left-over from copying and pasting.

> +  FnRetTy = CurFuncDecl->getType().getCanonicalType();
> +  FnRetTy = cast<FunctionType>(FnRetTy)->getResultType();
>
>
> This would be more idiomatic to write as:
>
> FnRetTy = CurFuncDecl->getType()->getAsFunctionType()- 
> >getResultType();

Fixed.  (This one is someone else's code - I just moved it, I didn't  
write it).

> +Value *ScalarExprEmitter::VisitObjCIvarRefExpr(ObjCIvarRefExpr *E) {
>
> [Aren't you doing something stupid there] (parahprased)

Yes.  Yes I am.  Fixed, I think.

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

This should not be very inefficient, since the loop will run a maximum  
of two times (for the two hidden arguments, unless someone writes  
another Objective-C runtime that uses more than two, which is possible  
without changing any of the non-runtime-specific code, but unlikely to  
happen).  In most cases it won't run at all, since the LocalDeclMap  
will give you the correct value.  If it doesn't, then it means that  
you've referred to self or _cmd (or self or _call in the Étoilé  
runtime).  In this case, then there is no decl for the argument (and  
it doesn't seem to be possible to easily create one from anything  
other than the parser), and so it checks the implicit arguments.  If  
you can add a constructor that lets a ParamDecl be created easily from  
the code generator then I'd be happy to use that instead (although I'm  
not 100% sure that would make sense, semantically).  As always,  
suggestions welcome...

Thanks for the feedback,

David

-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang.diff
Type: application/octet-stream
Size: 46251 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080326/a6776168/attachment.obj>
-------------- next part --------------