Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix Issue 22351 - extern(C++) function contravariant in D, but not C++ #14169

Merged
merged 4 commits into from
May 27, 2022

Conversation

ibuclaw
Copy link
Member

@ibuclaw ibuclaw commented May 25, 2022

Also fixes Issue 23135 - Covariance rules for C++ member functions mismatch D.

The special casing of mutable->const overrides in dtoh has been removed as with this change to the covariance rules for extern(C++), such a mismatch should never occur anymore.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @ibuclaw!

Bugzilla references

Auto-close Bugzilla Severity Description
22351 normal extern(C++) function contravariant in D, but not C++
23135 major Covariance rules for C++ member functions mismatch D

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14169"

@ibuclaw ibuclaw requested a review from MoonlightSentinel May 25, 2022 12:33
@ibuclaw ibuclaw marked this pull request as ready for review May 25, 2022 12:49
@ibuclaw ibuclaw force-pushed the issue23135 branch 2 times, most recently from 5134ecd to 6235eb6 Compare May 25, 2022 15:23
@tim-dlang
Copy link
Contributor

Thanks, this looks good.

The same problem also exists for parameters of functions. Here is a modified test case based on issue 22351:

extern(C++) class A
{
    int f(int*)
    {
        return 1;
    }
}

extern(C++) class B: A
{
    override int f(const(int)*)
    {
        return 2;
    }
}

extern(C++) B createB()
{
    return new B;
}
#include <stdio.h>

class A
{
public:
    virtual int f(int*);
};

class B: public A
{
public:
    virtual int f(const int*);
};

B *createB();

int main()
{
    B *b = createB();
    int i;
    printf("%d\n", b->f(&i));
    return 0;
}

Of course I could put it in a different issue, so it is not needed for this pull request.

@ibuclaw
Copy link
Member Author

ibuclaw commented May 26, 2022

@tim-dlang fix is in the same location, so can handle that here as well.

@ibuclaw ibuclaw force-pushed the issue23135 branch 2 times, most recently from a046608 to b3e0e4a Compare May 26, 2022 14:20
@ibuclaw
Copy link
Member Author

ibuclaw commented May 26, 2022

Switched this from error -> deprecation. This is mostly to allow the buildkite/ldc pipeline to succeed, but also to warn any downstream projects of the ABI change over a graceful period.

@ibuclaw ibuclaw force-pushed the issue23135 branch 2 times, most recently from b548a7e to 2618f1b Compare May 26, 2022 14:28
@tim-dlang
Copy link
Contributor

The following example could be a false positive for the deprecation:

extern(C++) class A
{
    int f()
    {
        return 1;
    }
    int f() const
    {
        return 2;
    }
}

extern(C++) class B: A
{
    alias f = A.f;
    override int f() const
    {
        return 3;
    }
}

void main()
{
    import std.stdio;
    A a = new B;
    writeln(a.f());
    const ac = a;
    writeln(ac.f());
}

The program currently prints 1 and 3, so B.f only overrides the overload A.f with const. DMD will now show the deprecation for B.f, but I think this example should still work.

@ibuclaw
Copy link
Member Author

ibuclaw commented May 26, 2022

@tim-dlang moved the deprecation out of the for loop, so it looks at the best match only, instead of every possible match.

@dlang-bot dlang-bot merged commit 7e1b115 into dlang:master May 27, 2022
@ibuclaw ibuclaw deleted the issue23135 branch May 27, 2022 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants