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

[David Chisnall]

On 1 Jun 2008, at 06:42, Chris Lattner wrote:

> On May 28, 2008, at 1:26 PM, David Chisnall wrote:
>>> +  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).
>
> If TypedSelectors are never used?  Why have them?

They aren't used yet, but they will be later (they allow a slightly  
more efficient implementation of Distributed Objects, which GNUstep  
supports).

> One simple way to do it with stringmap is to insert the string with  
> a \0 or some other sentinel between the two pieces.

I'll leave this for now, and come back to it when typed selectors are  
actually used.

>>> +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.
>
> Fair enough.  However, you don't have to copy and paste the structs,  
> just rewrite them based on your knowledge of what they are, for  
> example looking at the code that emits the LLVM IR and naming the  
> fields how you wish.
>
> +++ lib/CodeGen/CGObjCGNU.cpp	(working copy)
> +#include <cstdarg>
>
> Is this needed?

Ooops.  No, not now the helper functions have been moved back into LLVM.

> +const static int RuntimeVersion = 8;
> +static llvm::Constant *ProtocolVersion =
> +  llvm::ConstantInt::get(llvm::Type::Int32Ty, 2);
>
> RuntimeVersion is ok, but ProtocolVersion still requires a static  
> ctor.  Please remove.  It is also slightly more idiomatic to use  
> "static const int" instead of "const static int", but it doesn't  
> make that much of a difference.

I've turned ProtocolVersion into an int and now construct the  
llvm::ConstantInt where it's used.

> +static std::string SymbolNameForMethod(const std::string  
> &ClassName, const
> +    std::string CategoryName, const std::string &MethodName, bool  
> isClassMethod)
>
> CategoryName is still passed by copy

Fixed.

> +  if (llvm::Constant *CName =  
> llvm::dyn_cast<llvm::Constant>(SelName)) {
>
> I think you can just use dyn_cast instead of llvm::dyn_cast.  If  
> not, I suggest adding a 'using llvm::dyn_cast;' to the top of the  
> cpp file.

Fixed.

> +      llvm::GlobalAlias *Sel = new  
> llvm::GlobalAlias(llvm::PointerType::getUnqual(SelectorTy),
> +          llvm::GlobalValue::InternalLinkage,  
> ".objc_untyped_selector_alias", NULL,
> +          &TheModule);
>
> 80 cols, also elsewhere.

Fixed.

> Otherwise, looks great.  Please commit with these updates.  Thanks  
> David!

I've attached the new diff.  Can someone commit it?

David

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