Hello everyone,
If anyone has druntime + dmd + how context pointers work expertise, could you please help with reviewing this PR: https://github.com/dlang/druntime/pull/3638 ?
Thank you,
RazvanN
Thread overview | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
December 09, 2021 Help in reviewing druntime PR | ||||
---|---|---|---|---|
| ||||
Hello everyone, If anyone has druntime + dmd + how context pointers work expertise, could you please help with reviewing this PR: https://github.com/dlang/druntime/pull/3638 ? Thank you, |
December 09, 2021 Re: Help in reviewing druntime PR | ||||
---|---|---|---|---|
| ||||
Posted in reply to RazvanN | On Thursday, 9 December 2021 at 09:13:57 UTC, RazvanN wrote: >Hello everyone, If anyone has druntime + dmd + how context pointers work expertise, could you please help with reviewing this PR: https://github.com/dlang/druntime/pull/3638 ? Thank you, Like that even? oO I figured we'd just wait for @kinke to take a closer look :D ...I mean, it looks like this would have to be ported into Phobos as well, seeing as there's still this weird code duplication for those symbols going on. Though I would rather see those things removed from Phobos and aliased into core, deprecated, then deleted for good. But I could add the Phobos change too if there's more eyes there. |
December 09, 2021 Re: Help in reviewing druntime PR | ||||
---|---|---|---|---|
| ||||
Posted in reply to Stanislav Blinov | On Thursday, 9 December 2021 at 09:47:10 UTC, Stanislav Blinov wrote: >...I mean, it looks like this would have to be ported into Phobos as well, seeing as there's still this weird code duplication for those symbols going on. Though I would rather see those things removed from Phobos and aliased into core, deprecated, then deleted for good. But I could add the Phobos change too if there's more eyes there. Something like this is already in Phobos, under the name However, the behavior is not exactly the same. The Phobos version recurses into classes, even though they are reference types, leading to results like the following:
I'm not really sure what we should do about this. The Phobos behavior seems obviously incorrect to me, but "fixing" it is a potentially-breaking change. Maybe the right thing to do is to add the fixed version to Phobos as |
December 09, 2021 Re: Help in reviewing druntime PR | ||||
---|---|---|---|---|
| ||||
Posted in reply to Paul Backus | On Thursday, 9 December 2021 at 15:06:24 UTC, Paul Backus wrote: >On Thursday, 9 December 2021 at 09:47:10 UTC, Stanislav Blinov wrote: >...I mean, it looks like this would have to be ported into Phobos as well, seeing as there's still this weird code duplication for those symbols going on. Though I would rather see those things removed from Phobos and aliased into core, deprecated, then deleted for good. But I could add the Phobos change too if there's more eyes there. Something like this is already in Phobos, under the name However, the behavior is not exactly the same. The Phobos version recurses into classes, even though they are reference types, leading to results like the following:
I'm not really sure what we should do about this. The Phobos behavior seems obviously incorrect to me, but "fixing" it is a potentially-breaking change. Maybe the right thing to do is to add the fixed version to Phobos as This shan't be in Phobos, it needs to be in druntime. At the moment we have the whole gang of emplace, move, etc. duplicated between the two :\ These things need to reside in |
December 09, 2021 Re: Help in reviewing druntime PR | ||||
---|---|---|---|---|
| ||||
Posted in reply to Stanislav Blinov | On Thursday, 9 December 2021 at 15:52:46 UTC, Stanislav Blinov wrote: >This shan't be in Phobos, it needs to be in druntime. At the moment we have the whole gang of emplace, move, etc. duplicated between the two :\ These things need to reside in Yes, of course; I assumed that went without saying. My point is: we probably do not want to have duplicate implementations of this in two different places with slightly different behavior--which is where we're currently headed. |
December 09, 2021 Re: Help in reviewing druntime PR | ||||
---|---|---|---|---|
| ||||
Posted in reply to Paul Backus | On Thursday, 9 December 2021 at 18:04:26 UTC, Paul Backus wrote: >On Thursday, 9 December 2021 at 15:52:46 UTC, Stanislav Blinov wrote: >This shan't be in Phobos, it needs to be in druntime. At the moment we have the whole gang of emplace, move, etc. duplicated between the two :\ These things need to reside in Yes, of course; I assumed that went without saying. My point is: we probably do not want to have duplicate implementations of this in two different places with slightly different behavior--which is where we're currently headed. Yup, deduplication is next on the list! |
December 09, 2021 Re: Help in reviewing druntime PR | ||||
---|---|---|---|---|
| ||||
Posted in reply to Stanislav Blinov | On Thursday, 9 December 2021 at 15:52:46 UTC, Stanislav Blinov wrote: >This shan't be in Phobos, it needs to be in druntime. At the moment we have the whole gang of emplace, move, etc. duplicated between the two :\ These things need to reside in They've been moved from Phobos to druntime - but not 1:1, that's why the Phobos versions still exist. IIRC, the only reason is that the Phobos version additionally checks for self-references, which brings in an awful lot of code: https://github.com/dlang/druntime/pull/2442#issuecomment-452136871 I'd argue that self-references are invalid anyway (but not detected by the compiler) and problematic for implicit moves by the compiler - at least as long we don't have move ctors to fix up such self references. From https://dlang.org/spec/struct.html: >A struct is defined to not have an identity; that is, the implementation is free to make bit copies of the struct as convenient. Wrt. the review, please bear with me, I'm pretty busy at the moment. |