Fix _postblitRecurse for structs with private fields #1243
Conversation
|
Ping @monarchdodra, @MartinNowak. Would be nice to work out the dependency chain for dlang/phobos#3031 . |
|
Yeah, LGTM. Let's get this merged quickly. Unless I'm mistaken, 1181 basically broke any code that uses explicit destruction on structs with private fields? |
|
Destruction works fine as it uses tupleof, it is only postblit that is broken. |
| @@ -633,3 +633,41 @@ const(ubyte)[] toUbyte(T)(ref T val) if (is(T == struct) || is(T == union)) | |||
| return (cast(const(ubyte)*)&val)[0 .. T.sizeof]; | |||
| } | |||
| } | |||
|
|
|||
| char[] itoa(ulong n) pure nothrow @safe | |||
There was a problem hiding this comment.
This seems to be a duplicate of something in demangle.d:
https://github.com/D-Programming-Language/druntime/blob/5afecf8d7aa21f3269298767ec8d2c753103e6ba/src/core/demangle.d#L1706
It's implementation seems to be more efficient: It looks like you are writting the digits lsd to msd, and then reversing the digits? Instead, you can write the digits msd to lsd, and dup [20 - length .. length].
Bonus points for you though because you seem to handle the case where n == 0, whereas the demangle one does not seem to?
I get that returning a char[] means purity can make it a string, but it also means that you will always allocate for things such as "0". I guess it is moot here though.
Both functions are used differently, but would it be possible to consolidate the implementations somewhat?
There was a problem hiding this comment.
It is only used in CTFE, so I didn't see the GC-centric interface as an issue.
If core.demangle and object were to use the same implementation, would core.internal.convert still be a good place for it?
There was a problem hiding this comment.
It is only used in CTFE, so I didn't see the GC-centric interface as an issue.
Agreed. I was just pointing out a more general design issue, where an interface that returns mutable can't exploit immutability to return aliased results. As a random thought outside of this PR.
If core.demangle and object were to use the same implementation, would core.internal.convert still be a good place for it?
I have am not very savvy wrt how druntime is organized. @MartinNowak should be able to advise.
If the interfaces don't allow consolidation, or it is two difficult, then I'm fine with it. I would at the very least prefer that they have similar implementations.
Ah. Ok. I was wondering why we were not observing widespread destruction :) |
JakobOvrum
force-pushed
the
fix_object_recurse
branch
2 times, most recently
from
fefc25f
to
3191958
Compare
|
|
|
Ping @schveiguy, @MartinNowak. Got any time to look at this? |
| { | ||
| code ~= ` | ||
| _postblitRecurse(s.` ~ fieldName ~ `); | ||
| scope(failure) _destructRecurse(s. ` ~ fieldName ~ `); | ||
| _postblitRecurse(s.tupleof[` ~ itoa(i) ~ `]); |
There was a problem hiding this comment.
It appends a u for unsigned but apart from that it appears to work. If it can be relied on maybe we should use that instead.
|
You need to update the pull because we removed object.di. |
|
JakobOvrum
force-pushed
the
fix_object_recurse
branch
from
3191958
to
4c0224f
Compare
|
Now only modifies |
JakobOvrum
force-pushed
the
fix_object_recurse
branch
from
4c0224f
to
5318d24
Compare
|
Removed |
|
Auto-merge toggled on |
Fix _postblitRecurse for structs with private fields
Fixup for #1181