[cfe-dev] PATCH: Cleanup function redeclaration representations

[Chris Lattner]

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:
>
>    int f(int x) { return x; } // #1
>    int f(int x); // #2
>    int f(int x) { return x; } // #3
>
> That's an error (f is defined twice), but Clang doesn't diagnose it.
> If you swap the first two f's, or the last two f's, Clang does
> diagnose it properly.

Ah, this is an interesting problem and an interesting patch.

> In this patch, when  we notice that we're redeclaring an existing
> function, we merge the declarations together but *do not* introduce
> the new declaration into the scope (Argiris is doing the same thing
> with namespaces in his patch). That way, everyone who refers to a
> function "f" will refer to the same FunctionDecl, which will always be
> updated with all of the information we have about that function.

Ok.  Makes sense.

> Some notes:
>  - FunctionDecl::RedeclaredBy is used to update the main FunctionDecl
> with information from the newest FunctionDecl, and update the chain of
> previous declarations.
>
>  - FunctionDecl::getBody actually traverses the list of
> redeclarations to find the function definition. That way, we know
> *which* declaration is also a definition.

Ok.

>  - This change breaks CodeGen for the case where one sees the
> prototype of a function, generates a call to that function, and later
> sees the definition; see test/CodeGen/functions.c.
> CodeGenModule::GetAddrOfFunctionDecl currently relies on the later
> definition having a different FunctionDecl node (which used to be the
> case, but isn't with this patch). I can provide a follow-up patch to
> fix this issue, unless someone more familiar with the CodeGen module
> wants to deal with it.

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

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.


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


Please rename 'RedeclaredBy' as 'MarkRedeclaredBy' or something, to  
make it more verby.  Also, since it is mostly only useful during  
parsing time, should it be moved to Sema instead of living in the AST?


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

-Chris