Skip to content

Conversation

aG0aep6G
Copy link
Contributor

The destructor thing (14585) could probably be fixed more cleverly, without the additional space and indirection. But trying to be clever has lead to the issue in the first place.

The two overloads of get are effectively identical. The duplication could be eliminated by calling the mutable one from the const one, but with the duplication it's more robust against const-related bugs.

https://issues.dlang.org/show_bug.cgi?id=14585
https://issues.dlang.org/show_bug.cgi?id=14586

@aG0aep6G aG0aep6G force-pushed the variant branch 2 times, most recently from 181f5d8 to e723cf3 Compare May 15, 2015 13:34
@MartinNowak
Copy link
Member

Auto-merge toggled on

MartinNowak added a commit that referenced this pull request May 17, 2015
fix std.variant issues 14585 and 14586
@MartinNowak MartinNowak merged commit 4e0347a into dlang:master May 17, 2015
Buf buf;
memcpy(&buf, &info, info.sizeof);
static if (is(T == shared))
shared Unqual!T result;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think Variant should store shared data naively. For example it uses unlocked memcpy to move data around, which would fail if the data is shared and not atomic.

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 don't think Variant should store shared data naively. For example it uses unlocked memcpy to move data around, which would fail if the data is shared and not atomic.

I guess you're right. It's a different issue, though. And I'm not sure if I'd get the details right on this. Maybe file an issue or post on the forum?

@schuetzm
Copy link
Contributor

Help wanted: With this change, vibe.d no longer compiles (vibe-d/vibe.d#1115). Can anyone figure out whether it's vibe.d's fault, or Phobos', and what the problem is?

MartinNowak added a commit that referenced this pull request May 27, 2015
fixup PR #3284 - do a reinterpret cast
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