core.lifetime: Fix copyEmplace() wrt. shared/mutability qualifiers#3246
core.lifetime: Fix copyEmplace() wrt. shared/mutability qualifiers#3246dlang-bot merged 4 commits intodlang:masterfrom
Conversation
|
Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + druntime#3246" |
kinke
force-pushed
the
copyEmplace
branch
2 times, most recently
from
3560c98 to
1f32673
Compare
kinke
changed the title
src/core/lifetime.d
Outdated
| assert(t is s); | ||
|
|
||
| copyEmplace(si, ti); | ||
| assert(ti is si); |
There was a problem hiding this comment.
Any idea why this assertion above would fail for 32-bit x86 ELF debug (release works)? DMD codegen bug?
There was a problem hiding this comment.
Disabled for DMD x86 Posix now as workaround.
A tiny follow-up to dlang#3239. Trying to use it in std.variant showed this issue.
thewilsonator
left a comment
thewilsonator
left a comment
There was a problem hiding this comment.
Looks fine to me, cc @RazvanN7
| static if (__traits(isStaticArray, S)) | ||
| { | ||
| enum bool hasElaborateMove = hasElaborateMove!(typeof(S.init[0])); | ||
| enum bool hasElaborateMove = S.sizeof && hasElaborateMove!(BaseElemOf!S); |
There was a problem hiding this comment.
Any reason why length was changed to sizeof?
There was a problem hiding this comment.
I don't understand how BaseElemOf!S is any different from typeof(S.init[0])
There was a problem hiding this comment.
The difference shows up for multi-dimensional static arrays - BaseElemOf!(float[10][20][30]) == float. - Extracting it as little helper IMO helps to emphasize the intent and may reduce recursive instantiations of these hasElaborate{Move,Destructor,CopyConstructor,Assign} traits; higher dimensions don't matter here except for one of them being 0 and thus making the whole size 0; that's why I've had to change from recursively checking the length of each dimension to checking S.sizeof when passing a static array type and then instantiating the same template once again with the base element type.
There was a problem hiding this comment.
Makes total sense. Maybe we should have a test for this so that it does not get modified in the future.
There was a problem hiding this comment.
Yes, some tests added.
Corresponding to the front-end's Type.baseElemOf() method. Use it for some existing traits in that module, minimally fixing/ breaking semantics for `hasIndirections` for empty static arrays (now always false, independent from base element type).
RazvanN7
added
the
72h no objection -> merge
|
Given this is blocking other work I'm going to merge it now. |
thewilsonator
added
auto-merge
and removed
72h no objection -> merge
4 participants
A tiny follow-up to #3239. Trying to use it in
std.variantshowed these issues.