[cfe-dev] PATCH: Cleanup function redeclaration representations

[Doug Gregor]

On Sun, May 4, 2008 at 4:57 PM, Argiris Kirtzidis <akyrtzi at gmail.com> wrote:
> Doug Gregor wrote:
>
> > With Argiris' tweak, I almost like it. I'm concerned about the
> > programmability of this system, where the FunctionDecl node that is
> > found by name lookup is not the 'aggregate' node. As with the
> > canonical type system, clients will have to be very careful to always
> > map into the 'aggregate' node before querying any properties. With the
> > canonical type system, we need to do the mapping because we need to
> > preserve typedefs in the AST. With declarations, however, the
> > complexity isn't coming directly from the language... it's coming from
> > the representation. It will take a lot of discipline to use
> > FunctionDecls properly if name lookup doesn't find the 'aggregate'
> > FunctionDecl. (Among other things, we'll have to audit the whole Clang
> > code-base to see where we need to add GetAggregateDecl calls. Errors
> > here are likely to be subtle.).
> >
> >
>
>  That's a good point. I don't have a strong opinion on whether there should
> be a separate 'aggregate' node or not, but assuming that there is, can the
> semantic info for a function decl refer to the aggregate one for all redecls
> but for decl specific info you'll use separate methods ?

I think, from the programmer's standpoint, it doesn't matter so much
whether we have an 'aggregate' decl or not, so long as it looks like
we have an aggregate declaration.

However, if we have to reconstruct the contents of the aggregate
declaration every time we query the FunctionDecl, it's going to get
expensive because we'll be traversing the list of redeclarations and
accumulating information. (For example, think of merging attributes
every time we query an attribute!)

>  I mean, getbody() would return the body of the aggregate for all redecls,
> but you would also have a isThisDefinition() to probe the specific decl.
> Same for default arguments.
>  Is this practical ?

It's a little trickier for default arguments, because the default
argument is stored as part of ParmVarDecl rather than as part of the
FunctionDecl. The DefaultArgExpr trick can get around that specific
issue, of course.

Aside from that... it effectively doubles the size of the interface to
FunctionDecl (isInline and isThisInline, getAttrs and getThisAttrs,
etc.), but it does solve the issue without losing any source
information. It's harder to write bad code in this case, but one is
going to have to be very methodical when maintaining source
information to avoid writing isFoo rather than isThisFoo.

Overall, I think this approach is an improvement.

  - Doug