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 Issues 20714, 20965 - Postblit has priority over copy constructor #11945

Merged
merged 1 commit into from Nov 11, 2020

Conversation

RazvanN7
Copy link
Contributor

@RazvanN7 RazvanN7 commented Nov 11, 2020

Currently, whenever a postblit is defined, the copy constructor will get ignored (except if the postblit is disabled). This patch makes it so that whenever a copy constructor is defined it will take precedence over generated postblits. However if we have a user defined postblit, which is not disabled, it will take precedence over the copy constructor (user-defined or generated).

Technically this is a silent change of code behavior, however, I suspect that there are not any cases where you have copy constructors which are hanging around in code without being called. Anyway let's see if anything breaks.

This will need a spec update in case it will be accepted.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
20714 normal Struct with postblitting member does not call it's copy constructor
20965 major Implicitly generated postblit overrides disabled copy ctor

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#11945"

@dlang-bot dlang-bot merged commit c35ace6 into dlang:master Nov 11, 2020
@Geod24
Copy link
Member

Geod24 commented Nov 11, 2020

Common, what is it with slapping auto-merge a mere 4 minutes after the PR was submitted ?
This needs to come with a changelog entry for the behavior change and the spec change BEFORE it is merged.

Comment on lines +3 to +14
struct Blitter
{
int payload;
this(this){}
}

struct Adder
{
Blitter blitter;
this(int payload) {this.blitter.payload = payload;}
this(ref Adder rhs) {this.blitter.payload = rhs.blitter.payload + 1;}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way to make this more user friendly would be to print a deprecation message asking to explicitly disable the postblot in the parent struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that but making your struct non-copyable just because you want to use an instance of it as a field in a struct that has a copy constructor is not tractable (think of library code). Also there are cases like:

struct Blitter
{
    this(this) {}
}

struct Copy
{
    Blitter b;
    this(ref Copy c)
    {
         this.b = c.b;          // -> rely on postblit call
     }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One warning would work though: "Careful, you have fields with postblits! There will be no automatic generation of postblit for struct Copy and copy constructor will take precedence"

Copy link
Member

@Geod24 Geod24 Nov 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant was this:

struct Blitter
{
    this(this) {}
}

struct Copy
{
    Blitter b;
    @disable this(this); // If not present, deprecation message
    this(ref Copy c)
    {
         this.b = c.b;          // -> rely on postblit call
     }
}

This would work for library code, and avoid the silent change in behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From this comment [1]: "I don't want to further infest the codebase with postblits"

[1] https://issues.dlang.org/show_bug.cgi?id=20965#c3

Copy link
Member

@Geod24 Geod24 Nov 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well their code-base already uses postblit, event if that postblit comes from library code.
This changes breaks library code that relies on postblit, e.g.:

struct Foo
{
    int a;

    this(this)
    {
     	this.a++;
    }
}

struct Bar
{
    Foo f;
    int b;
    this(ref typeof(this) c)
    {
     	this.b++;
    }
}

void main ()
{
    Bar b;
    assert(b.f.a == 0);
    Bar c = b;
    assert(c.f.a == 1);
}

@Geod24
Copy link
Member

Geod24 commented Nov 11, 2020

Revert: #11947

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants