ObjC AST cleanup...

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

This seems entirely reasonable to me.

  - Doug

Sounds great to me Steve!

This would be a huge simplification! My next feature request would be to centralize and merge the various "lookup this thing in this class hierarchy" walks into some shared code.

-Chris

This will result in (a) less code, simplified classes. (b) allow us to
operate on method definitions more generically. (c) Improved naming
(ObjCImplementationDecl->ObjCClassImplDecl).

This would be a huge simplification!

Great. I'll do it tomorrow.

My next feature request would be to centralize and merge the various "lookup this thing in this class hierarchy" walks into some shared code.

I totally agree. I'll tackle this separately.

snaroff

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

This was previously, right? ObjCImplDecl has been removed.
Simplification is good though.

- Fariborz

Hi Steve,

Sorry for the late review...

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? 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.

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?

- Daniel

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).

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

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:-)

Makes sense. And I guess we don't want to call ObjCInterfaceDecl
ObjCClassDecl since that collides for our name for the @class node,
unless we renamed that to ObjCForwardClassDecl (which would, however,
be consistent with ObjCForwardProtocolDecl).

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...

Great, thanks for taking this on, I think it will make the ObjC AST
nodes much more pleasant to work with.

- Daniel