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

do not generate __fieldPostBlit if this(this) annotated with @disable #4073

Merged
merged 2 commits into from Oct 21, 2014

Conversation

IgorStepanov
Copy link
Contributor

@IgorStepanov
Copy link
Contributor Author

@monarchdodra This is a solution of the trouble, which we are discussed. Have you any objections?
I simply disabled generating __fieldPostBlit if this(this) annotated with @disable. This allows to define structs with const struct fields (however you should disable postblit for it).

@monarchdodra
Copy link
Contributor

I think you should file an issue so people that don't know what's going on would understand. I have no idea how dmd works though, so the usual dmd team will have to do the reviewing here.

@WalterBright
Copy link
Member

  1. this needs a bugzilla issue for it
  2. the fieldPostBlit should be disabled if any of the fields have disabled postblits
  3. the aggrPostBlit should be disabled if any of postblits[ ] are disabled.

@IgorStepanov
Copy link
Contributor Author

  1. this needs a bugzilla issue for it
  2. the fieldPostBlit should be disabled if any of the fields have disabled postblits
  3. the aggrPostBlit should be disabled if any of postblits[ ] are disabled.
  1. Done: see description.
  2. Done: stc variable accumulates storage classes of all field postblits.
  3. Done: already been so.

@@ -868,7 +868,9 @@ FuncDeclaration *buildPostBlit(StructDeclaration *sd, Scope *sc)
Loc loc = Loc(); // internal code should have no loc to prevent coverage

Expression *e = NULL;
for (size_t i = 0; i < sd->fields.dim; i++)
if (sd->postblits.dim && sd->postblits[0]->storage_class & STCdisable)
stc |= STCdisable;
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect.

  1. All of postblits[] should be tested for STCdisable
  2. All postblits used in fields should be tested for STCdisable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of postblits[] should be tested for STCdisable

Ok.

All postblits used in fields should be tested for STCdisable

It's done below.

stc = mergeFuncAttrs(stc, sd2->postblit);
if (stc & STCdisable)
{
     e = NULL;
     break;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of postblits[] should be tested for STCdisable

Done

{
stc |= STCdisable;
}

Copy link
Member

Choose a reason for hiding this comment

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

If any of postblits[] are disabled, then the field postblit should not be generated. I'd write the code above as:

for (size_t i = 0; i < sd->postblits.dim; ++i)
{
    stc |= sd->postblits[i]->storage_class & STCdisable;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. In which case there cam be many postblits in this point? I think this is situation where user defines postblits with different const modifiers:

struct Foo
{
    @disable this(this) const;
    this(this)
    {
         ...
    }
}

const Foo can not be copied, but we still may copy mutable Foo.
In this case we should generate __fieldPostBlit if at least one posthlit is not @disabled.
If user defines only one postblit, there will be only one element at this point.
Am I right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WalterBright I've added commit which follows your code. You may choose it or only the first commit. (See my explanations before). Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WalterBright Is it ok now? Are you sure that we should disable __fieldPostBlit is any of user postblits is disabled?

@WalterBright
Copy link
Member

If any of the fields cannot be copied, and any of the user supplied postblits cannot be used, then copying doesn't work so postblit should be disabled.

WalterBright added a commit that referenced this pull request Oct 21, 2014
do not generate __fieldPostBlit if this(this) annotated with @disable
@WalterBright WalterBright merged commit fdb99aa into dlang:master Oct 21, 2014
@IgorStepanov
Copy link
Contributor Author

Thanks

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