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 15604 - std.array.array of structs with template opAssign and default initialised 'new'ed class member #3952

Closed
wants to merge 1 commit into from

Conversation

John-Colvin
Copy link
Contributor

15604 is indicative of a larger problem, but this works around the issue here.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jan 26, 2016

Thanks for your pull request, @John-Colvin! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Fix Bugzilla Description
15604 [REG2.070] std.array.array of structs with template opAssign and default initialised 'new'ed class member

@John-Colvin John-Colvin force-pushed the patch-12 branch 4 times, most recently from 6f223f9 to 94159d3 Compare January 26, 2016 09:24
@@ -3967,7 +3967,7 @@ private void emplaceInitializer(T)(ref T chunk) @trusted pure nothrow
else
{
import core.stdc.string : memcpy;
static immutable T init = T.init;
static immutable T init;// = T.init;
Copy link
Member

Choose a reason for hiding this comment

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

This fails for types T with @disable this();, which seems to be the cause of the auto-tester failure

Copy link
Member

Choose a reason for hiding this comment

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

Maybe use TypeInfo.init

Copy link
Member

Choose a reason for hiding this comment

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

  • TypeInfo.init is deprecated and should be replaced by TypeInfo.initializer in all code that try to use it.
  • TypeInfo.... is not a good idea for such a core functuonality such as emplace, because it is not usable from version (FreeStanding) code.
  • T init = T.init is better than static immutable, because:
    • static immutable keeps objects alive for ever.
    • stack allocated data is more cache friendly

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, stack allocated structs are not allowed if they have @disable ~this();, so maybe we need to use static immutable at least for such types, probably under static if...

@dnadlinger
Copy link
Member

Why would this even be supposed to work in the first place? Isn't it clearly an accepts-invalid compiler issue that you can have a mutable member initialized to point to a class instance? Each struct instance would need to be initialized to point to a different object! If it were a static member, we could discuss whether it would make sense to put the class instance into TLS to support that use case, but not for per-object data.

Am I missing something here? What does the original use case look like?

@JakobOvrum
Copy link
Member

@klickverbot, member fields have compile-time initializers, and where but global/TLS would such data go? Putting them in global memory and TLS is consistent with every other compile-time initialized variable. When the programmer wants runtime initialization of fields, they can use a constructor.

When support for class types was initially added to compile-time initialized variables, it allowed all class types regardless of type qualifiers despite always allocating them in global memory. Since then, it has been made a compilation error to use compile-time initializers for mutable and unshared variables of class type at module-level and function scope. I assume the patch missed member fields.

@John-Colvin
Copy link
Contributor Author

I'd be happy to see this fixed at a language level, preferably in some way that disallows the madness of the test-case I'm using in this pull request.

@WalterBright WalterBright changed the title Fix issue 15604 Fix Issue 15604 - std.array.array of structs with template opAssign and default initialised 'new'ed class member Apr 15, 2016
@9il 9il added the Bug Fix label Jul 29, 2016
@dlang-bot dlang-bot added stalled and removed stalled labels Jun 22, 2017
@CyberShadow
Copy link
Member

Why would this even be supposed to work in the first place? Isn't it clearly an accepts-invalid compiler issue that you can have a mutable member initialized to point to a class instance?

Shall we close this and issue 15604 as invalid, then?

@John-Colvin
Copy link
Contributor Author

happy to close this, but I think 15604 should stay open, some sort of fix is necessary, whether it's documentation or better error messages or something more fundamental.

@John-Colvin John-Colvin closed this Jul 5, 2017
@John-Colvin John-Colvin deleted the patch-12 branch July 5, 2017 14:14
@CyberShadow
Copy link
Member

What do you think of refiling it as accepts-invalid (for mutable references to the data section it generates)?

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