[cfe-dev] PATCH: Cleanup function redeclaration representations

[Chris Lattner]

... catching up on clang, sorry for the delay...

On Apr 20, 2008, at 7:04 PM, Doug Gregor wrote:
>> 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.

Makes sense.

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

Sounds good.

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

Right.

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

Yuck, ok. :)

-Chris