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

[David Chisnall]

Hi Devang,

Thanks for the feedback.  Here is a new version.

On 27 May 2008, at 23:07, Devang Patel wrote:

>> +//FIXME: The capitalisation of methods in this class is horribly
>> inconsistent.
>
> This is not useful. In some sense, it encourages someone to be sloppy
> and hope that everything will be fixed!

Yup, total cop-out.  Fixed now.

>> +//FIXME Several methods should be pure virtual but aren't to avoid
>> the
>> +//partially-implemented subclass breaking.
>
> This is not specific. Remove this from here and add individual FIXMEs.

Ooops.  This is left over from earlier and is actually fixed in the  
code.  I've removed the comment.

>> +  virtual void GenerateCategory(const char *ClassName, const char
>> *CategoryName,
>> +             const llvm::SmallVector<llvm::Constant*, 16>
>> &InstanceMethodNames,
>
> If use
>   llvm::SmallVectorImpl<llvm::Constant *> &InstanceMethodNames
> then you do not hard code vector size here and let each runtime
> implementation use it appropriate size.

Thanks, I was wondering if there was a sensible way of doing this.   
I've fixed this in the interface.  There are a few places now in  
caller that can have their SmallVector sizes tuned a bit, but I'll  
wait for a later patch to do that.

>> +  virtual llvm::Value *generateMessageSendSuper(llvm::IRBuilder
>> &Builder,
>
> GenereateMessage...

Fixed.

>> +  virtual llvm::Value *getSelector(llvm::IRBuilder &Builder,
>
> GetSelector ...

Fixed.

>> isClassMethod)
>> +{
>> +  if (isClassMethod) {
>> +    return "._objc_method_" + ClassName +"("+CategoryName+")"+ "+"
>> + MethodName;
>> +  }
>
> nit pick. Use { } only if the block is more then 1 line.

I prefer to use blocks everywhere, since it makes it easier to see  
fall-through in nested conditionals and makes inserting debugging  
lines easier, but if LLVM coding conventions require them to not be  
blocks then I will try to remember in future.  I've removed this one.

>> +    std::vector<llvm::Constant*> V, std::string Name) {
>
> Please use vector reference here
>
>> +llvm::Constant *CGObjCGNU::MakeGlobal(const llvm::ArrayType *Ty,
>> +    std::vector<llvm::Constant*> V, std::string Name) {
>
> and here.

Fixed.

>>
>> +  llvm::Constant *C = llvm::ConstantArray::get(Ty, V);
>> +  return new llvm::GlobalVariable(Ty, false,
>> +      llvm::GlobalValue::InternalLinkage, C, Name, &TheModule);
>> +}
>> +
>> +/// Generate an NSConstantString object.
>> +//TODO: In case there are any crazy people still using Objective-C
>> without an
>> +//OpenStep implementation, this should let them select their own
>> class for
>> +//constant strings.
>
> :) Here, you want to say "GNU Objective-C runtime" here.

Pedantic, but since I am using this code with a Smalltalk compiler too  
I guess it's valid so I'll change it...
I stand by my statement that only crazy people program without  
OpenStep though :-)

>> +  if (ReturnTy->isFirstClassType() && ReturnTy !=
>> llvm::Type::VoidTy) {
>
> Now, isSingleType() is now preferred over isFirstClassType() here and
> other places where you check return type.

Fixed.  (Presumably this was meant to be isSingleValueType()?)

>> +    RetTy = ReturnTy;
>> +  } else {
>> +    // For struct returns allocate the space in the caller and pass
>> it up to
>> +    // the sender.
>
> Note, LLVM is moving in the direction to where return will be able to
> return aggregates.

Someone mentioned that C ABI issues make it difficult to use this in  
this particular case.  I have added a TODO to revisit it when LLVM  
gets support for aggregate return types.

>> +  for(unsigned int i=0 ; i<MethodTypes.size() ; i++) {
>
> for (unsigned int i = 0, e < MethodTypes.size(); i != e; ++i) {

Fixed everywhere I found - I might have missed one or two...


>> +  return MakeGlobal(ClassTy, Elements, SymbolNameForClass(Name-
>>> getStringValue()));
>> +}
> Add extra line before starting new function.

Fixed.

>> +  for(std::map<TypedSelector, llvm::GlobalAlias*>::iterator
>> +      iter=TypedSelectors.begin() ; iter!=TypedSelectors.end() ;
>> iter++) {
>
> Please use
>
> for(std::map<TypedSelector, llvm::GlobalAlias*>::iterator
>    iter=TypedSelectors.begin(), iterEnd =TypedSelectors.end();
>    iter != iterEnd; ++iter)
>
> form here and other places.

Fixed.  Also added the missing space between for and the bracket.

>> +void CodeGenModule::EmitObjCProtocolImplementation(const
>> ObjCProtocolDecl *PD){
>
> Why are these methods not runtime specific ?

These methods correspond to language features, which are runtime  
agnostic.  They then call the runtime-specific methods which perform  
the target-specific things.  All runtimes, for example, need to know  
the method names and type encodings in a protocol (and how these are  
found is specific to the source language, in this case Objective-C).   
This then calls the runtime method Runtime->GenerateProtocol, which  
either sets up some data structures or calls some runtime functions,  
depending on the runtime library being used.

>> +  llvm::SmallVector<std::string, 16> Protocols;
>> +  for(unsigned i=0 ; i<PD->getNumReferencedProtocols() ; i++)
>> +  {
>> +    Protocols.push_back(PD->getReferencedProtocols()[i]->getName());
>> +  }
>
> Avoid unnecessary { and }

Fixed here and in three other places.

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