Appender cannot add struct with immutable members

[monarch_dodra]

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.