[cfe-dev] Objective-C top-level constructs code generation
David Chisnall
csdavec at swansea.ac.uk
Sun Jun 1 06:08:36 PDT 2008
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>
More information about the cfe-dev
mailing list