On Friday, 8 March 2013 at 20:19:48 UTC, Namespace wrote:
AFAIK, the problem is actually within emplace, that tries to
call opAssign when it should really be just (post)blitting.
This is just one of the many emplace-related bugs.
I have an open pull for fixing emplace that should fix this.
I'll add your snippet to its unittests to confirm that it also
fixes this issue.
You said in my bug report:
There is a lot of broken in appender that I've tried to fix
before, but it is
very tricky because:
What exactly is broken?
First, let me say it turns out it is not emplace related (but it
should be).
The issues are basically in 2 families:
1) Individual append: Basically, appender always just adds
elements by doing an opAssign. This is wrong because, as you have
noticed, the type may simply not be assignable. Furthermore,
appending should entail blitting,not assgining. Finally, Because
appender requests un-initialized memory, calling a complex
opAssign on non-initialized data is undefined behavior.
The solution should be to simply call emplace, but as I've said,
we must preserve CTFE and performance, and currently, I'm not
sure emplace does either (though I have an open pull that
should). Even when emplace gets fixed, there is stil the
potential overhead of a non-inlined function call for when doing
basic type appending.
2) (Re) localtion: Two minor famililes.
2.1) Mixing arrays and GC.malloc. This makes it so that there are
a few areas of contention: For example, calling expend on an
array slice means the returned "capacity" may not actually match
the array capacity. Furthermore, it means that the data appender
is operating on may or may not be T.init initialized. This may or
may not be a problem, but appender is not consistent in the way
it deals with this. Further more if and when arrays finalize,
appender will leak if it uses raw allocations.
2.2) When relocating, appender does a straight up memcpy. This
would be fine if T didn't have a postblit, but when it does, it
means you are duplicating something without calling the adequate
code. This is wrong, and usually catastrophic when the
destructors get called. Lucky for us, currently, arrays don't
finalize.
The correct solution could be either of
a) Always call posblit (costly)
b) memcopy and then reset the original payload to T.init
(cheaper). The probem with this approach is that if somebody
already has a handleon payload, you are essentially wipping data
from him. This is especially problematic if the data was tagged
as immutable, as that would be a violation of immutable, no
matter how you interpret the documentation of appender.
I still prefer the b) solution, but with the caveat that types
with postblit need a bool to keep track if or if not there is an
"outside world view" of the payload, in which case it would
postblit.
That's about it. As you can see, part of the problem is that
fixing it correctly also implies making new design choices, and
not just straight up correcting code.
