[cfe-dev] PATCH: C++ Function Overloading (v2)

[Doug Gregor]

On Wed, Oct 22, 2008 at 11:53 AM, Argiris Kirtzidis <akyrtzi at gmail.com> wrote:
> Doug Gregor wrote:
>>
>>
>>>
>>> So what is the advantage of using
>>> - CallExpr -> DeclRefExpr -> OverloadedFunctionDecl (unique for this
>>> expression)
>>> instead of
>>> - CallExpr -> OverloadedFunctionExpr
>>> ?
>>>
>>> The second avoids the ownership issues of the first (plus it's a bit more
>>> compact).
>>>
>>
>> The advantage of the first is that name lookup has something it can
>> return when it finds a set of overloaded functions. Name lookup is
>> going to find a Decl of some sort, and when that Decl is really a set
>> of overloaded function declarations, we need a Decl to represent it.
>> Hence, we have OverloadedFunctionDecl to refer to those declarations.
>>
>
> Ok, I think I see the main usefulness of OverloadedFunctionDecl.
> LookupDecl returns a Decl; the reasoning is to keep it that way and deal
> with multiple overloaded FunctionDecls by having LookupDecl return a single
> OverloadedFunctionDecl. The callers of LookupDecl can still reason of name
> lookup as returning a single Decl.
> While there's a bit of a cost involved, it certainly makes things cleaner.

Yep. And I think we can eliminate most of the cost.

> About the current implementation:
> OverloadedFunctionDecl is created in "anticipation" of a possible name
> lookup and not as the result of "executing" a name lookup (and it doesn't
> seem to have an owner). Isn't it better to have OverloadedFunctionDecl be
> created by LookupDecl and have it represent the result of a particular name
> lookup search ?
>
> e.g: as in the using directive example:
>
> namespace A {
> void f(int); #1
> }
>
> void f(char); #2
>
> void g() {
> using namespace A; #3
> }
>
> With the current implementation, a OverloadedFunctionDecl should be created
> when #3 is encountered, just in case there's a lookup for 'f' later on. Then
> when 'g()' is exited, the OverloadedFunctionDecl should be discarded and the
> FunctionDecl be made to take its place.
> If the OverloadedFunctionDecl is created only when needed (when there's a
> name lookup of 'f' involved) no OverloadedFunctionDecl will be created in
> this case (which is the optimal thing to do).

Right. Although, since we don't actually support using directives
right now, we can't really say how it handles them :)

> Another advantage is that using this way, the ownership issue of
> OverloadedFunctionDecl can be handled in a clean way, like this:
> -OverloadedFunctionDecls returned by LookupDecl are newly created objects
> not owned by anyone. The caller creates a OverloadedFunctionDeclRefExpr
> expression that holds and owns this specific OverloadedFunctionDecl object.
> -OverloadedFunctionDeclRefExpr is a subclass of DeclRefExpr and the main
> difference is that it owns the referenced Decl.
> -When the overloads get resolved (in non-template code),
> OverloadedFunctionDeclRefExpr is removed and destroyed, which in turn
> destroys the OverloadedFunctionDecl that it references.
> -For templates, OverloadedFunctionDeclRefExpr remain in the AST.
>
> What do you think ?

Generally, I like it, although I think we can do things a bit more
efficiently. I'd like the DeclContext to own the
OverloadedFunctionDecls, and cache them so that we aren't constantly
re-creating them. This means that, for example, the first lookup of
"f" for an overloaded function would find a number of FunctionDecl
nodes, for which we'd create an OverloadedFunctionDecl. At that point,
we should push the OverloadedFunctionDecl into the scope, so that it
can be found and reused. This would be a lazy form of what the code is
doing now.

In the case of a template, where we need to store the
OverloadedFunctionDecl for later use, I suggest that we have the code
that needs to save the OverloadedFunctionDecl make a copy of it that
will be owned by the DeclRefExpr or OverloadedFunctionDeclRefExpr, so
that we're not wasting time creating and destroying
OverloadedFunctionDecls that will only be used once before being
destroyed.

Another minor comment: we could probably eliminate
OverloadedFunctionDeclRefExpr and instead use the lowest bit of the
NamedDecl* in DeclRefExpr to record whether the DeclRefExpr owns the
declaration or not.

  - Doug