Skip to content

Fix issue 16142: Disabled opAssign is overriden when a dtor/postblit is present#5854

Merged
WalterBright merged 1 commit intodlang:masterfrom
mathias-lang-sociomantic:opassign-2
Jul 1, 2016
Merged

Fix issue 16142: Disabled opAssign is overriden when a dtor/postblit is present#5854
WalterBright merged 1 commit intodlang:masterfrom
mathias-lang-sociomantic:opassign-2

Conversation

@mathias-lang-sociomantic
Copy link
Contributor

In order to pick up disabled opEquals, we need to process the members
of the struct anyway even if there is a dtor / postblit.

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
16142 Adding a dtor / postblit (even disabled) forces opAssign


// One of our sub-field might have `@disable opAssign` so we need to
// check for it.
for (size_t i = 0; i < sd.fields.dim; i++)
Copy link
Member

Choose a reason for hiding this comment

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

Use foreach (i; 0 .. sd.fields.dim) or foreach (field; sd.fields) (if it works).

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'm merely moving things around and adding documentation here.
I have further plans for this part of the code so I'll keep this comment in mind for later.

@mathias-lang-sociomantic
Copy link
Contributor Author

Depends on dlang/phobos#4421

@mathias-lang-sociomantic mathias-lang-sociomantic changed the title Fix issue 16142: Disabled opEquals is overriden when a dtor/postblit is present [Don't merge] Fix issue 16142: Disabled opEquals is overriden when a dtor/postblit is present Jun 14, 2016
@mathias-lang-sociomantic
Copy link
Contributor Author

Alright, there are a couple of other issues with opAssign which I want to address before pulling this in.

@mathias-lang-sociomantic mathias-lang-sociomantic changed the title [Don't merge] Fix issue 16142: Disabled opEquals is overriden when a dtor/postblit is present Fix issue 16142: Disabled opEquals is overriden when a dtor/postblit is present Jun 20, 2016
@mathias-lang-sociomantic
Copy link
Contributor Author

It's green.

@WalterBright @andralex : Pinging you for language change.

Currently, opAssign is not 100% transitive, so a struct Foo containing a struct Bar will not call its field opAssign if Foo has a destructor or posblit (source).

According to the specs, an elaborate opAssign is needed if the struct has either a destructor, a postblit, or a member with an elaborate opAssign. By reading it one might easily see transitiveness being implied, but it isn't, and the spec don't actually say if its members opAssign are called by the generated opAssign or not. AFAICS that "field with elaborate opAssign requirement was put in place in order for destructors to be called.

However, as it was recently outlined on the forums by 2 different users ( @deadalnix here and Eyal from Weka), one might want to use postblit + identity opAssign to ensure consistency or avoid unwanted copy/move, which is not possible at the moment.

As a result, this P.R. is a first step in opAssign propagation, which starts by propagating @disable even though the @disabled method wouldn't have been called by the compiler.

@WalterBright
Copy link
Member

Why does this say it's about opEquals when it's about opAssign? Please fix!

Loc loc = Loc(); // internal code should have no loc to prevent coverage

// One of our sub-field might have `@disable opAssign` so we need to
// check for it.
Copy link
Member

Choose a reason for hiding this comment

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

Might want to add that the purpose of the next block of code is to set stc.

@mathias-lang-sociomantic mathias-lang-sociomantic changed the title Fix issue 16142: Disabled opEquals is overriden when a dtor/postblit is present Fix issue 16142: Disabled opAssign is overriden when a dtor/postblit is present Jun 24, 2016
@mathias-lang-sociomantic
Copy link
Contributor Author

Why does this say it's about opEquals when it's about opAssign? Please fix!

Oops, fixed.

@mathias-lang-sociomantic
Copy link
Contributor Author

@WalterBright : Just to be on the safe side: are you okay with the plan outlined here ? :)

…is present

In order to pick up disabled opEquals, we need to process the members
of the struct anyway even if there is a dtor / postblit.
@mathias-lang-sociomantic
Copy link
Contributor Author

mathias-lang-sociomantic commented Jul 1, 2016

Updated:

diff --git a/src/clone.d b/src/clone.d
index ddf72cc..8d1899b 100644
--- a/src/clone.d
+++ b/src/clone.d
@@ -209,6 +209,8 @@ extern (C++) FuncDeclaration buildOpAssign(StructDeclaration sd, Scope* sc)

     // One of our sub-field might have `@disable opAssign` so we need to
     // check for it.
+    // In this event, it will be reflected by having `stc` (opAssign's
+    // storage class) include `STCdisabled`.
     for (size_t i = 0; i < sd.fields.dim; i++)
     {
         VarDeclaration v = sd.fields[i];
  • rebased on master

@WalterBright : Ping on #5854 (comment)

@WalterBright
Copy link
Member

Auto-merge toggled on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants