[cfe-dev] PATCH: Cleanup function redeclaration representations

Doug Gregor doug.gregor at gmail.com
Sun Apr 20 19:04:02 PDT 2008


On Sun, Apr 20, 2008 at 4:13 PM, Chris Lattner <clattner at apple.com> wrote:
> On Apr 20, 2008, at 9:15 AM, Doug Gregor wrote:
> > While working on some of the preliminaries for overloading, I stumbled
> > across an issue with the way Clang represents redeclarations of
> > functions. The basic problem is that, although Clang recognizes
> > redeclarations, it does not represent them as such. Here's a simple
> > little example that can be used to illustrate the problem:

>  I can fix the codegen part after you commit this patch, no worries, just
> let the test fail.

Thanks!

>  Some minor thoughts:
>
>  What is the intended ownership model of the previous declarations?  I see
> that your patch has functiondecl delete other definitions when the
> functiondecl is deleted, is this intended, or do you think TranslationUnit
> should own the previous declarations?  Is the intent that a client would
> walk a declaration list for the translation unit and only see each function
> once... walking the prev declaration list to see other declarations?  This
> model makes sense to me, but it would be nice to explicitly say this
> somewhere in a prominent comment.

Yes, this was the intent. Since all of the function declarations
represent the same function, I think it makes the most sense for that
function to only have one entry in the translation unit's list of
declarations, because there really is only one function being
declared. Those (probably few) clients that want to know about *all*
declarations of a function can walk the previous-declaration chains.

>  It is a little strange to me that getBody() can return non-null if
> isDefinition() return false.  How about renaming getBody() -> findBody() and
> having getBody() just return the local function?

I think perhaps the name of isDefinition is really the cause of the
problem here, and that's the name we should change. I've gone with the
*very* explicit isThisDeclarationADefinition, because this is a
special-purpose question about (essentially) the ordering of the
re-declarations. I only expect it to be used in a few diagnostics,
where we want to say "f is (declared|defined) here".

I think splitting getBody into findBody() (which walks the tree) and
getBody() will end up being confusing. Most of the time, all we care
about is "is there a definition for this function?" Having two
functions to answer that question (which produce the same answer
except in very weird cases; see below) is going to cause clients
trouble.

> Also, is there ever a case
> where getBody could ever return a FunctionDecl other than 'this'?  I thought
> definitions were always at the front of the list.

Surprisingly, no. One can actually write, e.g.,

  void f(int x) { }
  void f(int x = 5);

>  Please rename 'RedeclaredBy' as 'MarkRedeclaredBy' or something, to make it
> more verby.

"AddRedeclaration". Given that Argiris is doing the same thing for
namespaces, we should consider moving this up the Decl hierarchy and
making it virtual.

> Also, since it is mostly only useful during parsing time,
> should it be moved to Sema instead of living in the AST?

I feel like this is an important part of the consistency of the AST.
By keeping this member as a part of the FunctionDecl it allows the
members of FunctionDecl to remain protected, and (I hope) it will be
far easier to remember to keep this function in sync with changes to
FunctionDecl.

>  Overall, the patch looks great.  Please feel free to commit when you're
> happy with the above issues,

Thanks! Done:

  http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080414/005376.html

  - Doug



More information about the cfe-dev mailing list