www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Help in reviewing druntime PR

reply RazvanN <razvan.nitu1305 gmail.com> writes:
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
Dec 09 2021
parent reply Stanislav Blinov <stanislav.blinov gmail.com> writes:
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,
 RazvanN


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.
Dec 09 2021
parent reply Paul Backus <snarwin gmail.com> writes:
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 
[`std.traits.hasNested`][1].

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:

```d
void example()
{
     import std.traits;

     class Nested {}
     static struct NotNested
     {
         Nested nested;
     }

     pragma(msg, hasNested!NotNested); // true
}
```

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 
`std.traits.hasContextPointers` and mark `hasNested` as 
outdated/unrecommended in the documentation. Thoughts?

[1]: https://dlang.org/phobos/std_traits.html#hasNested
Dec 09 2021
parent reply Stanislav Blinov <stanislav.blinov gmail.com> writes:
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 
 [`std.traits.hasNested`][1].

 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:

 ```d
 void example()
 {
     import std.traits;

     class Nested {}
     static struct NotNested
     {
         Nested nested;
     }

     pragma(msg, hasNested!NotNested); // true
 }
 ```

 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 
 `std.traits.hasContextPointers` and mark `hasNested` as 
 outdated/unrecommended in the documentation. Thoughts?

 [1]: https://dlang.org/phobos/std_traits.html#hasNested


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 `core.lifetime` 
and threabout, usable without Phobos. Phobos could public alias 
if need be, but certainly not the other way around :)
Dec 09 2021
next sibling parent reply Paul Backus <snarwin gmail.com> writes:
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 
 `core.lifetime` and threabout, usable without Phobos. Phobos 
 could public alias if need be, but certainly not the other way 
 around :)


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.
Dec 09 2021
parent Stanislav Blinov <stanislav.blinov gmail.com> writes:
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 
 `core.lifetime` and threabout, usable without Phobos. Phobos 
 could public alias if need be, but certainly not the other way 
 around :)


 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!
Dec 09 2021
prev sibling parent reply kinke <noone nowhere.com> writes:
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 
 `core.lifetime` and threabout, usable without Phobos. Phobos 
 could public alias if need be, but certainly not the other way 
 around :)


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.
Dec 09 2021
parent kinke <noone nowhere.com> writes:
On Thursday, 9 December 2021 at 21:02:23 UTC, kinke wrote:

 self-references


Read: interior pointers.
Dec 09 2021