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

[Chris Lattner]

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?


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


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)


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


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




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



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




+    Elements.push_back(MakeConstantString((*iter).first.first,  
".objc_sel_types"));

Watch out for 80 columns.



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


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

This is looking nice!

-Chris