-
-
Notifications
You must be signed in to change notification settings - Fork 706
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 std.variant issues 14585 and 14586 #3284
Conversation
181f5d8 to
e723cf3
Compare
|
Auto-merge toggled on |
fix std.variant issues 14585 and 14586
| Buf buf; | ||
| memcpy(&buf, &info, info.sizeof); | ||
| static if (is(T == shared)) | ||
| shared Unqual!T result; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
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? |
fixup PR #3284 - do a reinterpret cast
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