Issue 2789 - Functions overloads are not checked for conflicts

[yebblies]

Two issues here:

• The check in overloadInsert is disabled as it can't work until semantic has been run on all overloads. This has been changed so that the check and the actual insertion can be run seperately.
• The check is out of date. Type::covariant is no longer suitable, as it allows a much wider set of conversions and requires return type covariance, which is not needed for two overloads to conflict. The new function to perform this check is TypeFunction::overloadConflict.

http://d.puremagic.com/issues/show_bug.cgi?id=2789

[MartinNowak]

You already do check for mod == mod in overloadConflict.
Also testing isFuncAliasDeclaration is much cheaper and should be done
before calling overloadConflict.

[yebblies]

Will do.

[MartinNowak]

I'd prefer a separate function for the check.

void FuncDeclaration::overloadCheck()
{
    for (FuncDeclaration *f = overnext; f; f = f->overnext)
    {
        // do the check
    }
}

Then you can call f->overloadCheck() before overloadInsert where you had set OVERcheck now.

This is also a good opportunity to get rid of that pointless recursion in overloadInsert where all we want
to do is appending to the end.

[yebblies]

I'd rather keep them as one function. If I split them up, most of the code will end up duplicated for not a lot of gain. There is plenty of precedent for using flags like this.

I don't see what the issue with the recursive call is. I'd prefer to change as little code as possible with each patch, in my experience it reduces conflicts and aids reviewing.

I especially don't want to change the flow used in this function because it also involves alias and template overloads. I will have a look at them after this patch is merged, but I'd rather not do too much at once here.

[MartinNowak]

Ah your right, overloadInsert is polymorph and will switch between different functions.
Doesn't that mean you'd need to make AliasDeclaration::overloadInsert and
TemplateDeclaration::overloadInsert aware of these flags too?

Some unittests that check overloading of functions, aliases and templates would be nice, not sure what works though.

[MartinNowak]

Why do I have a conflict if one type is NULL and the other is not?

[yebblies]

Typo. And probably useless code anyway, I think I'll replace it with assert(!null)

[yebblies]

Doesn't that mean you'd need to make AliasDeclaration::overloadInsert and TemplateDeclaration::overloadInsert aware of these flags too?

No, their behavior doesn't need to change. The only modification was to the big test which only applied to functions, the others are handled in other paths. There are problems with aliases and templates (I've got the issue numbers written down somewhere) but I'd like to handle them in a separate patch.

Some unittests that check overloading of functions, aliases and templates would be nice, not sure what works though.

Not much. Overloading with aliases is iirc completely unchecked, and overloading with templates is completely broken.

[andralex]

LGTM pending final review by @dawgfoto.

[andralex]

please rebase

[MartinNowak]

This has to be coordinated with #1409 and #1660.

[yebblies]

TBH this is so old I don't remember how it works. It will likely need to be redone after @9rnsr's template overloading pulls are merged anyway, so I'm going to close it.