Fix Issue 19830 - core.memory.__delete destructs arrays of structs in the wrong order #2585
Conversation
|
Thanks for your pull request and interest in making D better, @JinShil! 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
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "stable + druntime#2585" |
|
Thanks! |
| @@ -1067,7 +1067,7 @@ void __delete(T)(ref T x) @system | |||
| { | |||
| static if (is(E == struct)) | |||
| { | |||
| foreach (ref e; x) | |||
| foreach_reverse (ref e; x) | |||
There was a problem hiding this comment.
See https://github.com/dlang/druntime/blob/master/src/rt/lifetime.d#L1157 for what currently happens with delete: finalizers are only called if it is a runtime allocated array (debatable), pointer offset is corrected for large arrays (GC.free is wrong otherwise), block cache is invalidated (not doing so might make later operations unsafe).
There was a problem hiding this comment.
That seems to be an existing issue with __delete's impementation, but out of scope of this PR. Can you just file a bug report, so it can be dealt with properly?
There was a problem hiding this comment.
Probably we should just have __delete call _d_delarray_t.
There was a problem hiding this comment.
Can't do that as these C functions are supposed to go once the deprecation is over.
There was a problem hiding this comment.
That seems to be an existing issue with
__delete's impementation, but out of scope of this PR.
Fine with me.
Can you just file a bug report, so it can be dealt with properly?
https://issues.dlang.org/show_bug.cgi?id=13558
Probably we should just have
__deletecall_d_delarray_t.
That's what I thought, too.
There was a problem hiding this comment.
I think what @rainers is saying is the implementation is still needed; it just doesn't get called directly by the compiler. We either need to move the implementation into __delete or have __delete call it. I suppose we could also convert _d_delarray_t into a template to get rid of the dependency on TypeInfo.
There was a problem hiding this comment.
I think Andrei would want us to move the implementation into __delete as the general idea is to move into a D without any runtime.
There was a problem hiding this comment.
Ok, I'll look into that soon.
This issue was discovered while working on dlang/dmd#9666
This test https://github.com/dlang/dmd/blob/b0007c1193dfb72bcfa8cef3f420fee68564ce49/test/runnable/sdtor.d#L186-L208 does not pass when
deleteis replaced with__delete. This PR fixes that.