[cfe-dev] Objective-C top-level constructs code generation

[David Chisnall]

On 28 May 2008, at 17:27, Chris Lattner wrote:

> On May 28, 2008, at 8:56 AM, David Chisnall wrote:
>> Hi Devang,
>>
>> Thanks for the feedback.  Here is a new version.
>
> Nice!  This is looking good.  A few more thoughts:
>
> +// Some zeros used for GEPs in lots of places.
> +static  llvm::Constant *Zeros[] =
> {llvm::ConstantInt::get(llvm::Type::Int32Ty, 0),
> +      llvm::ConstantInt::get(llvm::Type::Int32Ty, 0) };
> +static llvm::Constant *NULLPtr = llvm::ConstantPointerNull::get(
> +    llvm::PointerType::getUnqual(llvm::Type::Int8Ty));
> +static llvm::Constant *ProtocolVersion =
> +  llvm::ConstantInt::get(llvm::Type::Int32Ty, 2);
>
> This causes static initializers to be formed and run.  Please stay
> away from them.  Maybe these should be instance variables of the  
> class?

Okay.

> +  std::map<std::string, llvm::Constant*> ExistingProtocols;
> +  typedef std::pair<std::string, std::string> TypedSelector;
> +  std::map<TypedSelector, llvm::GlobalAlias*> TypedSelectors;
> +  std::map<std::string, llvm::GlobalAlias*> UntypedSelectors;
>
> std::map's from std::string are really inefficient.  Can you use
> StringMap for these?

Done for the two with string keys.  I'm happy to accept suggestions  
for a more efficient way of storing typed selectors (although clang  
currently never uses that code path, so the inefficiency isn't  
important).

> These:
> +static std::string SymbolNameForClass(std::string ClassName) {
> +static std::string SymbolNameForMethod(const std::string &ClassName,
> const
> +    std::string CategoryName, const std::string MethodName, bool
> isClassMethod)
>
>
> Copy the std::string objects.  Please pass as "const std::string &foo"
> to avoid this.  Likewise in a few other places (e.g. MakeGlobal)

Done.

> +  if (isClassMethod)
> +    return "._objc_method_" + ClassName +"("+CategoryName+")"+ "+" +
> MethodName;
> +  return "._objc_method_" + ClassName +"("+CategoryName+")"+ "-" +
> MethodName;
> +}
>
> How about something like:
>   return "._objc_method_" + ClassName +"("+CategoryName+")"+
>         (isClassMethod ? "+" : "-") + MethodName;
>
> To make it more obvious what the difference is between the two.

Yup, seems clearer.

> +  if (ReturnTy->isSingleValueType() && ReturnTy !=
> llvm::Type::VoidTy) {
> +    llvm::SmallVector<llvm::Value*, 16> Args;
> +    Args.push_back(Receiver);
> +    Args.push_back(cmd);
> +    Args.insert(Args.end(), ArgV, ArgV+ArgC);
> +    return Builder.CreateCall(imp, Args.begin(), Args.end());
> +  } else {
> +    llvm::SmallVector<llvm::Value*, 16> Args;
> +    llvm::Value *Return = Builder.CreateAlloca(ReturnTy);
> +    Args.push_back(Return);
> +    Args.push_back(Receiver);
> +    Args.push_back(cmd);
> +    Args.insert(Args.end(), ArgV, ArgV+ArgC);
> +    Builder.CreateCall(imp, Args.begin(), Args.end());
> +    return Return;
> +  }
>
> Is there a specific reason not to share the common code here?

Other than incompetence?  No.  Fixed.


> +void CGObjCGNU::GenerateProtocol(const char *ProtocolName,
>
> It would be nice to add a block comment above each of these methods
> with a C struct (in the comment) that describes the thing that you are
> generating.  This would make it easier to follow the code.

I don't want to copy the structs directly from the runtime headers,  
since the header is GPL'd (with a special exemption for code compiled  
with GCC, but not for code compiled with LLVM) and, although you can't  
copyright an interface, you can copyright a representation of an  
interface (see AT&T Vs UCB).  I've added a comment at the top of the  
file pointing people in the right direction for finding the structures.

> +  if (Classes.size() + Categories.size() + ConstantStrings.size() +
> +      ExistingProtocols.size() + TypedSelectors.size() +
> +      UntypedSelectors.size() == 0) {
>
> It is generally better to query .empty() instead of .size() if you
> just care about whether it is empty or not.  Also, using "if
> (x.empty() && y.empty() && z.empty() ...)" is more idiomatic than
> using additions.

Fixed.

> +    Elements.push_back(MakeConstantString((*iter).first.first,
> ".objc_sel_types"));
>
> Watch out for 80 columns.

Fixed.

> +  const llvm::StructLayout *Layout =
> +    TheTargetData.getStructLayout(llvm::cast<const
> llvm::StructType>(ObjTy));
>
>
> You don't need the llvm:: qualifier on cast, and you don't need
> 'const' in the type argument (the constness of the result follows the
> constness of the input).  This should be enough:
>
> +  const llvm::StructLayout *Layout =
> +    TheTargetData.getStructLayout(cast<llvm::StructType>(ObjTy));

Done.

> +++ lib/CodeGen/ModuleBuilder.cpp	(working copy)
> @@ -67,6 +67,24 @@
>
>        if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
>          Builder->EmitFunction(FD);
> +      } else if (dyn_cast<ObjCClassDecl>(D)){
> +        //Forward declaration.  Only used for type checking.
>
> If you're not using the result of the dyn_cast, please use
> isa<ObjCClassDecl>(D) instead.  Likewise for other cases in this file.

Fixed.

David

-------------- next part --------------
A non-text attachment was scrubbed...
Name: objc.diff
Type: application/octet-stream
Size: 61969 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080528/8337f69f/attachment.obj>
-------------- next part --------------