[cfe-dev] PATCH: Cleanup function redeclaration representations

[Argiris Kirtzidis]

Doug Gregor wrote:
> Perhaps the real issue is that we shouldn't be calling
> HandleTopLevelDecl for re-declarations, because they aren't declaring
> unique entities, e.g., HandleTopLevelRedeclaration.
>   

There are types of ASTConsumers that would expect to get re/declarations 
as the parser encounters them. Think of a consumer that colorizes 
variable/param names
for an IDE. If we hide the redeclarations we make things complicated for 
them, I'm not sure what the benefit would be.

>   
>>  How about instead of linking with previous decl, linking with next decl is
>> used instead. For example:
>>
>>
>>   int f(int x) { return x; } // #1
>>   int f(int x); // #2
>>   int f(int x) { return x; } // #3
>>
>>
>>  -For #1 , parser reports it
>>  -For #2, it is merged with #1, #2 is not introduced in scope, #1 points to
>> #2 (as next decl), and parser reports the decl pointer of #2
>>  -For #3, it is merged with #1, #3 is not introduced in scope, #2 points to
>> #3 (as next decl), and parser reports the decl pointer of #3
>>
>>  I think this is simpler and there's no need to have in AST the
>> addDeclaration method that swaps the contents of decl objects.
>>  The drawback is that there needs to be an additional connection, like #2
>> and #3 both pointing to #1 as the decl that represents the function.
>>
>>  What do you think ?
>>     
>
> I had a patch that was a little like this, with one difference: once
> #1 was merged into #2 (then #2 into #3), I removed the previous
> declaration from the scope and pushed the new declaration. That way,
> subsequent lookups of the function get the latest declaration. This is
> really important, both for semantic and usability reasons:
> semantically, we need all of the information from later declarations
> in the FunctionDecl that name lookup finds, e.g.,
>
>   void f(int x); // #1
>   void f(int x = 2); // #2
>   void g() { f(); } // need to see the default argument from declaration #2
>
> The same thing happens in C, if the first declaration doesn't have a prototype.
>
> So, if name lookup is going to continue to find the first declaration
> of a function (as it does in your suggestion and in the patch I
> committed), that first declaration either needs to accumulate
> information from redeclarations or access to its internals needs to
> walk through the chain of redeclarations, e.g., to find default
> arguments.
>   

Yes, that's what I'm proposing. The first decl accumulates the 
information and/or walks the redeclarations for info.
Like the getBody() in your patch that walks the redeclarations to get 
the body.

> The comment I have about this approach is that we'll end up with
> different calls to the same function pointing at different
> FunctionDecl nodes. For example:
>
>   void f(int x); // #1
>   void g(int x) { f(x); } // call points to #1
>   void f(int x); // #2
>   void h(int x) { f(x); } // call points to #2
>
> I don't know whether that's a benefit or not :) In some sense, it
> accurately represents the source code (that's good), but it's also a
> little confusing: they both refer to exactly the same function, so why
> do they refer to different FunctionDecl nodes?
>   

I don't quite understand what you mean. Name lookup should only see and 
return the first decl (as the function decl that represents 'f' function).
Both calls should point to #1, why will they refer to different 
FunctionDecl nodes ?

> There's some philosophy behind my approach to function redeclarations:
> I want "normal" users of the AST to only see a single FunctionDecl for
> each function, and that FunctionDecl should contain all of the
> information that the AST contains about that function.

I agree. The only difference is in case of

  void f(int x); // #1
  void f(int x = 2); // #2


instead of merging to #2 and swaping contents with #1, thus pointer of 
#1 now points to #2,
I suggest merging to #1.
In both cases, every use of 'f' will point to the same FunctionDecl node.

That way ASTConsumers like the syntax-colorizer I mentioned and 
pretty-printer (SerializationTest uses this one), can use redeclarations 
the same way
as single declarations. If a ASTConsumer cares only about single decls, 
it can check a decl.isRedeclaration() method and act accordingly.


-Argiris