[cfe-dev] ObjC AST cleanup...

[steve naroff]

On Dec 18, 2008, at 9:05 PM, Daniel Dunbar wrote:

> Hi Steve,
>
> Sorry for the late review...
>

No problem.

> This sounds great Steve, this would simplify some code in the IRgen
> for Objective-C as well.
>
> One simple question, would it be better to call ObjCClassImplDecl
> ObjCInterfaceImplDecl?

It's unclear if this is better. @interface is used for both classes  
and categories (so ObjCInterfaceImplDecl is more ambiguous).

 From my perspective, ObjCClassImplDecl is more precise. We could have  
ObjCClassInterfaceImplDecl and ObjCCategoryInterfaceImplDecl, however  
that's getting a little verbose/bizarre:-)

> The code is inconsistent with using "class" and
> "interface" and I think we should clean this up.
>
> Somewhat more complicated, how does this all relate to the current set
> of Objective-C container objects we have right now?
>
> Here is a brief taxonomy of the Objective-C container objects:
>
> Currently there are five:
> (1) ObjCInterfaceDecl
> (2) ObjCCategoryDecl
> (3) ObjCProtocolDecl
> (4) ObjCImplementationDecl
> (5) ObjCCategoryImplDecl
>
> These container objects all act as containers for class and instance
> methods with associated accessors (instmeth_*, classmeth_*).
>
> ObjCCategoryImplDecl and ObjCImplementationDecl use a SmallVector for
> the method arrays, the others just use a pointer to an array.
>
> ObjCCategoryImplDecl and ObjCImplementationDecl don't have property
> lists (classprop_*), the other three do.
>
> My main question is could we make something even higher than an
> ObjCImplDecl which implemented the "method container" abstraction? It
> looks to me like this would be straightforward and would simplify the
> code a lot.
>

I think this makes sense.

> Beyond that, it may also be worth having a common class for
> implementing the "property container" abstraction.
>
> This would yield the following hierarchy:
>
> ObjCContainerDecl
> \--> ObjCPropertyContainerDecl (FIXME: this is a horrible name)
> \-->---> ObjCCategoryDecl
> \-->---> ObjCInterfaceDecl
> \-->---> ObjCProtocolDecl
> \--> ObjCImplDecl
> \-->---> ObjCCategoryImplDecl
> \-->---> ObjCClassImplDecl
>
> For simplicities sake, if we went this route it might make sense to
> just merge ObjCPropertyContainerDecl into ObjCContainerDecl (even
> though ObjCImplDecl wouldn't actually use the property list). We don't
> expect to actually have many implementations in any given translation
> unit so I think the simplicity in the class hierarchy might be a good
> tradeoff for a little extra memory usage.
>
> What do you think?
>

I think using containers for methods and properties makes sense  
(though I need to think a bit more about ObjCPropertyContainerDecl...I  
agree it would be nice to eliminate it).

I hope to get around to this type of cleanup soon...

Thanks for the feedback/review,

snaroff

> - Daniel
>
> On Tue, Dec 2, 2008 at 11:41 AM, steve naroff <snaroff at apple.com>  
> wrote:
>> To folks that work with the ObjC AST's,
>>
>> I've recently written some annoying code that needs to deal
>> (generically) with method definitions (attached to a class or  
>> category).
>>
>> As a result, I'd like to consider adding a new base class
>> ObjCImplDecl, that will contain the methods and properties for a  
>> class
>> or category.
>>
>> Here is the new class hierarchy I'm considering:
>>
>> class ObjCImplDecl : NamedDecl // new
>> class ObjCClassImplDecl : ObjCImplDecl  // previously
>> 'ObjCImplementationDecl : NamedDecl'
>> class ObjCCategoryImplDecl : ObjCImplDecl // previously
>> 'ObjCCategoryImplDecl : NamedDecl'
>>
>> This will result in (a) less code, simplified classes. (b) allow us  
>> to
>> operate on method definitions more generically. (c) Improved naming
>> (ObjCImplementationDecl->ObjCClassImplDecl).
>>
>> Since changes to the AST's can effect everyone, I wanted to get some
>> buy-in before I make this change.
>>
>> Anyone see any problems with this?
>>
>> Thanks,
>>
>> snaroff
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>