[cfe-dev] PATCH: Cleanup function redeclaration representations

[Doug Gregor]

Hi Argiris,

On Wed, Apr 23, 2008 at 11:24 AM, Argiris Kirtzidis <akyrtzi at gmail.com> wrote:
>  When a function is redeclared, the parser reports the same decl pointer to
> the ASTConsumer, basically the ASTConsumer gets a new object but with the
> same
>  pointer as the previous decl. I think that this complicates things a bit
> from the aspect of a ASTConsumer.
>
>  For example, there's a consumer in clang (SerializationTest used with
> '-test-pickling'), that stores the decls reported from HandleTopLevelDecl
> into a vector
>  so that it can later 'feed' them in the same order to another consumer.
> This breaks when a redeclaration is reported because it then stores the same
> pointer
>  for each redeclaration.

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.

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

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?

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. Advanced
clients that need to know about re-declarations can then walk the
redeclaration chain from that one FunctionDecl.

  - Doug