ZDoom

Moved this to Code Submissions since it has one.

Once it's updated, I'll merge it into QZDoom for testing.

Okay then, [url=https://github.com/coelckers/gzdoom/pull/967]here it is[/url]. I hope I haven't overlooked anything, as it didn't seem to be that difficult to implement. If I have... well, at least I tried. :P

Yes, they should always match the current version.

Okay. Should I also increase the version number in GZDoom's built-in script files? I did some tests yesterday, and there weren't any compilation errors.

Yes, that needs to be versioned. I'd consider that case incorrect, but if it passed the compiler it needs to remain operational for old script versions.

class A
{
    virtual C GetSomething()
    {
        ...
    }
}

class B : A
{
    D GetSomething() // Note: not virtual
    {
        ...
    }
}

class C
{
}

class D : C
{
}

Okay, that other PR has been resolved, but there is still something important that has to be considered.

Take a look at the following ZScript:
[code]class A
{
virtual C GetSomething()
{
...
}
}

class B : A
{
D GetSomething() // Note: not virtual
{
...
}
}

class C
{
}

class D : C
{
}[/code]
As of now, this code compiles without errors. However, if the changes to implement this feature, they way I've coded it, are introduced, compilation will fail because of D::GetSomething missing the override qualifier.

This can be mitigated with a ZScript version bump. PClass::FindVirtualIndex could use the old, exact-matching algorithm to check return values in the old version, and the new algorithm to allow covariant return types in the new version.

If there are any objections to this particular issue, then I will probably give up on the PR, since otherwise it will require more extensive changes to ZCCCompiler::CompileFunction with a much greater risk to break something unintentionally. But then, I hope, it could eventually be done later by someone more experienced in this matter.

I will wait with the PR until [url=https://github.com/coelckers/gzdoom/pull/966/]this one[/url] is resolved, since there might be conflicts.

[quote="phantombeta"]IMO this is not a good idea. Virtual functions should not be modifiable like this. As much as I like things that make it easier to code and catch bugs, I really don't think something like this should be added.[/quote]
C++ allows covariant return types. This feature is pretty useful in real world programs. So called virtual constructor is one of its applications.

I'm not sure if there is any actual danger in adding this, and it might make code easier to read and understand. In fact, I think I already know how to implement it, and was preparing a PR when you wrote this... But there may be some subtle pitfalls I'm not seeing, since I only resumed coding in ZScript a week ago. I'm also currently developing an Android app in Java at work, and I know that Java is one such language that has covariant return types, and there were some use cases where they came in handy. In my current ZScript project I have a similar use case, and that's why I remembered about this feature.

IMO this is not a good idea. Virtual functions should not be modifiable like this. As much as I like things that make it easier to code and catch bugs, I really don't think something like this should be added.

class Base1
{
    virtual Base2 GetSomething()
    {
        return null;
    }
}

class Child1 : Base1
{
    override Child2 GetSomething() // Script error: Attempt to override non-existent virtual function GetSomething
    {
        return null;
    }
}

class Base2
{
}

class Child2 : Base2
{
}

[b]Current behavior:[/b]

When overriding a virtual function in a subclass, the overridden function must have the exact same return type as the one the parent virtual function does. For example, the following ZScript will not compile:

[code]class Base1
{
virtual Base2 GetSomething()
{
return null;
}
}

class Child1 : Base1
{
override Child2 GetSomething() // Script error: Attempt to override non-existent virtual function GetSomething
{
return null;
}
}

class Base2
{
}

class Child2 : Base2
{
}[/code]

[b]Suggested new behavior:[/b]

Allow the return type of the overridden function to be either the same as, or a subtype of the parent function's return type.

[b]Justification:[/b]

Imagine I'm working with an instance of Child1. If I needed to make use of the extra functionality of the type Child2 that is returned by Child1::GetSomething, the only solution would be to cast the returned Base2 to Child2. This breaks encapsulation and is also dangerous, because if Child1::GetSomething is changed to return something other than a Child2, the cast will return null, and a VM abort could be triggered.

However, if Child1 could advertise right away that its GetSomething returns not just a Base2 but actually a Child2, then changing the return type would trigger a compile error, which could make it easier to avoid potential bugs.