Fix Issue 23337 - Wrongly elided postblit/copy ctor for array construction (_d_arrayctor lowering) #14442
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 references
|
| // only non-trivial array constructions may need to be lowered (non-POD elements basically) | ||
| Type t1e = t1b.nextOf(); | ||
| TypeStruct ts = t1e.baseElemOf().isTypeStruct(); | ||
| if (!ts || (!ts.sym.postblit && !ts.sym.hasCopyCtor && !ts.sym.dtor)) |
There was a problem hiding this comment.
Isn't this just the same logic as isPOD?
There was a problem hiding this comment.
Almost, and I don't really like this 'optimization' either: #14372 (comment) - especially after noticing yesterday that the same bug fixed in that PR (missing check for copy ctor) still holds for array assignments:
dmd/compiler/src/dmd/expressionsem.d
Line 10020 in fe2b56e
| if (!ts || (!ts.sym.postblit && !ts.sym.hasCopyCtor && !ts.sym.dtor)) | ||
| return setResult(res); | ||
| // don't lower ref-constructions etc. | ||
| if (!(t1b.ty == Tsarray || ae.e1.isSliceExp) || |
There was a problem hiding this comment.
The tests currently only test the static-array LHS case. I'm not sure about this isSliceExp check - do we really have ConstructExp with a slice[] LHS?
There was a problem hiding this comment.
int[4] b = [1, 2, 3, 4];
struct A
{
int[4] a;
this(int c)
{
a[0 .. 4] = b;
}
}Technically, a[0 .. 4] = b; should be a construct expression.
There was a problem hiding this comment.
Yes, even a[] = b is a construct expression. This is what isSliceExp is for.
| // If ae.e2 is not a variable, construct a temp variable, as _d_arraysetctor requires `ref` access | ||
| if (!ae.e2.isVarExp) | ||
| // promote an rvalue RHS element to a temporary, it's passed by ref to _d_arraysetctor | ||
| if (!ae.e2.isLvalue) |
There was a problem hiding this comment.
[This change is tested in line 66 (S[3] fromSingleLval = *p;), getting rid of an extra copy if the RHS is a non-var lvalue.]
|
@kinke why is this still a draft? Looks ready to me. |
|
I'm on vacation and will get back to this, adding a test for the slice-exp lhs. |
|
Okay, added the slice-expression LHS tests - which immediately showed a little regression in my previous logic, triggered by earlier implicit casts from static arrays to slices - so I'm afraid the overall logic doesn't seem to get any more elegant... |
|
~~ This is a bit risky to merge without the buildkite tester, although phobos passing is quite encouraging. ~~ Later edit: I restarted the buildkite failure, fingers crossed. |
…ction (_d_arrayctor lowering)
Which required further logic tweaks to prevent previously invisible regressions for two such corner cases - wrong _d_arrayctor lowerings with array literals and static-array rvalues, the latter implicitly cast to slices.
|
Thanks @kinke ! |
No description provided.