[cfe-dev] Objective-C: methods, ivars, and cleanup

[Devang Patel]

On Mar 26, 2008, at 8:09 AM, David Chisnall wrote:

>> In this method you do a field lookup by iterating over all ObjectTy
>> ivars. Would not it be more efficient to keep a map just like  
>> normal C
>> struct fields ?
>
> You'd need to generate the map at some point, possibly the first  
> time it's called?

The map is collected for C structure while determining its layout (in  
ConvertType()).

>  Since it works as-is, I'd rather do this later if profiling  
> indicates this is a bottleneck.

A class is defined once but its fields are referenced multiple times,  
so it makes sense to avoid lookups during field access. That's what we  
do for C structures. Let's not diverge here.

> +  assert(!verifyFunction(*CurFn));

nit-pick, add a message here.


>>>> +    // Warning: Use of this is strongly discouraged.  Late binding
>>>> of instance
>>>> +    // variables is supported on some runtimes and so using static
>>>> binding can
>>>> +    // break code when libraries are updated.  Only use this if
>>>> you have
>>>> +    // previously checked that the ObjCRuntime subclass in use
>>>> does not support
>>>> +    // late-bound ivars.
>>>>
>>>> Please add assert() to check late-bound of ivars support.
>>>
>>> I'd like to do this, but GodeGenTypes does not have a reference to
>>> anything that has a reference to anything that has a reference to
>>> the ObjCRuntime subclass, so it's non-trivial.  Suggestions welcome.
>>
>> You may want to collect runtime info while constructing CodeGenTypes.
>
> Doing this would require major changes to the structure of  
> CodeGenModule, since its Types member would have to be initialised  
> after the runtime was constructed.  It's also not quite semantically  
> correct - you should still be able to get the static structure of  
> the object using @defs() at compile time, and the compiler should  
> emit a warning that you are introducing stronger constraints on the  
> ABI that the runtime requires (and thus susceptible to the fragile  
> base class problem).

hmm.. OK for now.

Otherwise patch looks OK. Very nice!
Thanks,
-
Devang