www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.learn - Appender cannot add struct with immutable members

reply "Namespace" <rswhite4 googlemail.com> writes:
Is this intended behavior or just a bug / missing feature?
http://dpaste.1azy.net/21f100d3

I ask because it would be very easy to solve, with memcpy / 
memmove.
http://dpaste.1azy.net/ac2b38e5 (line 192)
Mar 04 2013
next sibling parent "bearophile" <bearophileHUGS lycos.com> writes:
On Monday, 4 March 2013 at 13:14:43 UTC, Namespace wrote:

 Is this intended behavior or just a bug / missing feature?


It's not intended.

Bye,
bearophile
Mar 04 2013
prev sibling next sibling parent Marco Leise <Marco.Leise gmx.de> writes:
Am Mon, 04 Mar 2013 14:14:42 +0100
schrieb "Namespace" <rswhite4 googlemail.com>:


 Is this intended behavior or just a bug / missing feature?
 http://dpaste.1azy.net/21f100d3
 
 I ask because it would be very easy to solve, with memcpy / 
 memmove.
 http://dpaste.1azy.net/ac2b38e5 (line 192)


There are several issues with immutable/const fields in
structs. It seems this idea wasn't explored enough in the
early designs of immutable. Often you need to express that the
left-hand side of an assignment is 'fresh' and it doesn't
harm immutability if it is assigned to.

-- 
Marco
Mar 04 2013
prev sibling parent reply "monarch_dodra" <monarchdodra gmail.com> writes:
On Monday, 4 March 2013 at 13:14:43 UTC, Namespace wrote:

 Is this intended behavior or just a bug / missing feature?
 http://dpaste.1azy.net/21f100d3

 I ask because it would be very easy to solve, with memcpy / 
 memmove.
 http://dpaste.1azy.net/ac2b38e5 (line 192)


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.
Mar 08 2013
parent reply "Namespace" <rswhite4 googlemail.com> writes:
 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?
Mar 08 2013
parent reply "monarch_dodra" <monarchdodra gmail.com> writes:
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.
Mar 08 2013
parent reply "Namespace" <rswhite4 googlemail.com> writes:
Wow, very large. I'm going to read this tomorrow in a rested 
state in more detail. But it arouses my interest to write 
something yourself from scratch.
But a quick question: Why is the internal array in the separate 
struct 'Data' encapsulated? To enable a performant postblit, 
since only a pointer need to be copied? If so, I thought the 
nested struct must be static?
Mar 08 2013
parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Friday, 8 March 2013 at 22:59:23 UTC, Namespace wrote:

 Wow, very large. I'm going to read this tomorrow in a rested 
 state in more detail. But it arouses my interest to write 
 something yourself from scratch.
 But a quick question: Why is the internal array in the separate 
 struct 'Data' encapsulated? To enable a performant postblit, 
 since only a pointer need to be copied? If so, I thought the 
 nested struct must be static?


AFAIK, it is to have refernce semantics on copy: EG: To pass an 
appender by value, but still have observable modifications. Its 
basically the "hand written class implementationvia structs" 
semantic.

Note though that it does have the fatal flaw that if the Appender 
is not initialized (allocated payload), then you won't actually 
see the modifications if you immediatly pass it by value (eg: use 
appender as a sink for format, for example). But this is more of 
a "D-wide" issue. I recommend always initializing your appender 
to (at least) a null array: It is not useful in and out of 
itself, but forces payload initialization.

----
Declaring the struct as static enforces that it doesn't have a 
context pointer. However, if it is not declared as static, and 
doesn't need a context pointer, it won't have one. the keyword 
static is an "opt-in" keyword. It is also very under-used in 
phobos, but I think it is good practice to use it as much as 
possible.

----
I've tried to write an array re-implementation, but it is 
delicate work. Up until now, I've been fixing or improving more 
minor things. That and the fact that very few of my pulls seem to 
actually go through and just pile up, I'm trying to not burdern 
the reviewers with a complicated development.

Maybe I should be more pro-active and getting my stuff reviewed?
Mar 09 2013