I had been using the idiom you describe, but upon looking into it, it's for reasons that no longer apply. Appender used to have a release() method that gave you access to the underlying array, but insisted on reallocating it under certain circumstances before it did. Now that this "feature" is gone, I wouldn't mind too much seeing the unsafe pointer constructor go.<br>
<br>In general, though, since Appender is entirely a performance hack anyhow, it should err on the side of performance, not safety. I think there needs to be an @system way of initializing from an existing array and stomping issues be damned.<br>
<br><div class="gmail_quote">On Wed, Aug 25, 2010 at 3:48 PM, Steve Schveighoffer <span dir="ltr"><<a href="mailto:schveiguy@yahoo.com">schveiguy@yahoo.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Alright, I have now a working Appender to replace the current one (along with<br>
the appropriate changes to phobos).<br>
<br>
This one should be completely usable in safe mode save a couple of functions<br>
(clear and shrinkTo).<br>
<br>
Before I commit, I want to get the opinions of others on this:<br>
<br>
One very very notable difference from the original appender is it does not take<br>
a pointer as the parameter. This means it does not update the original array<br>
you pass to it (well, it does, but will not modify the length). There was only<br>
really a couple places in phobos that used this to an advantage, most places<br>
that used appender had code like this:<br>
<br>
string buf;<br>
auto app = appender(&buf);<br>
<br>
And then never used buf again.<br>
<br>
So I'm wondering how much this feature is needed (affecting the original<br>
array). I feel its a very unsafe method of passing an array around, especially<br>
when we have perfectly safe ways, and the updated version of the array is<br>
available via the data property.<br>
<br>
Other than that, here is the updated API (with private data shown for an idea of<br>
how the implementation works):<br>
<br>
struct Appender(A : T[], T)<br>
{<br>
private struct Data<br>
{<br>
size_t capacity;<br>
Unqual!(T)[] arr;<br>
}<br>
<br>
private Data* _data;<br>
<br>
/**<br>
Construct an appender with a given array. Note that this does not copy the<br>
data, but appending to the array will copy the data, since Appender does not<br>
know if this data has valid data residing after it. The initial capacity will<br>
be<br>
arr.length.<br>
*/<br>
this(T[] arr);<br>
<br>
/**<br>
Reserve at least newCapacity elements for appending. Note that more elements<br>
may be reserved than requested. If newCapacity < capacity, then nothing is<br>
done.<br>
*/<br>
void reserve(size_t newCapacity);<br>
<br>
/**<br>
Returns the capacity of the array (the maximum number of elements the<br>
managed array can accommodate before triggering a reallocation). If any<br>
appending will reallocate, capacity returns 0.<br>
*/<br>
@property size_t capacity();<br>
<br>
/**<br>
Returns the managed array.<br>
*/<br>
@property T[] data();<br>
<br>
/**<br>
Appends one item to the managed array.<br>
*/<br>
void put(U)(U item) if (isImplicitlyConvertible!(U, T) ||<br>
isSomeChar!T && isSomeChar!U);<br>
<br>
/**<br>
Appends an entire range to the managed array.<br>
*/<br>
void put(Range)(Range items) if (isForwardRange!Range<br>
&& is(typeof(Appender.init.put(items.front))));<br>
<br>
/**<br>
Clears the managed array. This leaves the capacity intact.<br>
*/<br>
void clear();<br>
<br>
/**<br>
Shrinks the managed array to the given length. Passing in a length<br>
that's greater than the current array length throws an enforce exception.<br>
*/<br>
void shrinkTo(size_t newlength);<br>
<div class="im">}<br>
<br>
<br>
<br>
<br>
----- Original Message ----<br>
> From: Steve Schveighoffer <<a href="mailto:schveiguy@yahoo.com">schveiguy@yahoo.com</a>><br>
> To: Discuss the phobos library for D <<a href="mailto:phobos@puremagic.com">phobos@puremagic.com</a>><br>
</div><div><div></div><div class="h5">> Sent: Wed, August 25, 2010 9:50:37 AM<br>
> Subject: Re: [phobos] I think we need to make an emergency release<br>
><br>
><br>
><br>
> OK, looked at it for a while, I wasn't clear originally on what Appender is<br>
> exactly supposed to do.<br>
><br>
> I found a superficial bug before I wrote the message below -- you were<br>
>stomping<br>
><br>
> over the capacity without rewriting it.<br>
><br>
> But in general, I think the method you are using to store the capacity is<br>
> flawed. It's prone to stomping (i.e. using appender on a slice) and it's<br>
> somewhat inefficient (you are constantly re-storing the capacity). It's going<br>
><br>
> to run into problems if you try using ~= on an array that you appended to with<br>
><br>
> Appender.<br>
><br>
> Part of the problem is that Appender is trying to apply its metadata to an<br>
> existing array. Appender is using data it doesn't own or isn't given<br>
>permission<br>
><br>
> to change. This can be bad on many levels (including violating immutability<br>
> rules).<br>
><br>
> I'd recommend scrapping Appender as written and creating one that does the<br>
> following:<br>
><br>
> 1. if initialized with an existing array, initialize capacity to 0, forcing a<br>
<br>
> realloc on first append<br>
> 2. Store the capacity outside the array data (i.e. in a heap-allocated<br>
> array+capacity struct).<br>
><br>
> Yes, 1 can be inefficient but it errs on the side of safety. Once it<br>
> reallocates the first time, it will be very efficient. This works as long as<br>
<br>
> the assumption is that you use Appender to append large amounts of data.<br>
><br>
> I'll work on a proposed replacement Appender struct, it shouldn't take too<br>
>long.<br>
><br>
> -Steve<br>
><br>
><br>
> ----- Original Message ----<br>
> > From: Andrei Alexandrescu <<a href="mailto:andrei@erdani.com">andrei@erdani.com</a>><br>
> > To: Discuss the phobos library for D <<a href="mailto:phobos@puremagic.com">phobos@puremagic.com</a>><br>
> > Sent: Wed, August 25, 2010 9:09:33 AM<br>
> > Subject: Re: [phobos] I think we need to make an emergency release<br>
> ><br>
> > Thanks, and also thanks for announcing this. I was about to start<br>
> > looking at it...<br>
> ><br>
> > Andrei<br>
> ><br>
> > On 8/25/10 6:08 PDT, Steve Schveighoffer wrote:<br>
> > > I think I know the problem with the code. I'll check in a fix, and update<br>
><br>
> >the<br>
> > > bug, see if it helps.<br>
> > ><br>
> > > -Steve<br>
> > ><br>
> > ><br>
> > ><br>
> > > ----- Original Message ----<br>
> > >> From: Don Clugston<<a href="mailto:dclugston@googlemail.com">dclugston@googlemail.com</a>><br>
> > >> To: Discuss the phobos library for D<<a href="mailto:phobos@puremagic.com">phobos@puremagic.com</a>><br>
> > >> Sent: Tue, August 24, 2010 3:35:09 PM<br>
> > >> Subject: [phobos] I think we need to make an emergency release<br>
> > >><br>
> > >> David suggested we need an emergency release for the array appender<br>
> > >> bug, and I'm inclined to agree with him. My personal experience is<br>
> > >> that it makes 2.048 unusable, and I've had to go back to 2.047. Here's<br>
> > >> the bug:<br>
> > >><br>
> > >> Bug 4681 Appender access violation<br>
> > >> This is a really horrible memory corruption regression, caused by svn<br>
> > >> 1813. Is there any reason we shouldn't just roll that back?<br>
> > >><br>
> > >> ---<br>
> > >> BTW, one compiler bug popped up in the last release, too. Fairly<br>
> > >> obscure though. I created a patch:<br>
> > >> 4655 Regression(1.063, 2.048) goto to a try block ICEs<br>
> > >><br>
> > >> I also made a patch for this compiler bug from three releases back:<br>
> > >> 4302 Regression(1.061, 2.046) compiler errors using startsWith in<br>
>CTFE<br>
> > >><br>
> > >> All other known compiler regressions were introduced in 2.041 or<br>
> earlier.<br>
> > >> _______________________________________________<br>
> > >> phobos mailing list<br>
> > >> <a href="mailto:phobos@puremagic.com">phobos@puremagic.com</a><br>
> > >> <a href="http://lists.puremagic.com/mailman/listinfo/phobos" target="_blank">http://lists.puremagic.com/mailman/listinfo/phobos</a><br>
> > >><br>
> > ><br>
> > ><br>
> > ><br>
> > > _______________________________________________<br>
> > > phobos mailing list<br>
> > > <a href="mailto:phobos@puremagic.com">phobos@puremagic.com</a><br>
> > > <a href="http://lists.puremagic.com/mailman/listinfo/phobos" target="_blank">http://lists.puremagic.com/mailman/listinfo/phobos</a><br>
> > _______________________________________________<br>
> > phobos mailing list<br>
> > <a href="mailto:phobos@puremagic.com">phobos@puremagic.com</a><br>
> > <a href="http://lists.puremagic.com/mailman/listinfo/phobos" target="_blank">http://lists.puremagic.com/mailman/listinfo/phobos</a><br>
> ><br>
><br>
><br>
><br>
> _______________________________________________<br>
> phobos mailing list<br>
> <a href="mailto:phobos@puremagic.com">phobos@puremagic.com</a><br>
> <a href="http://lists.puremagic.com/mailman/listinfo/phobos" target="_blank">http://lists.puremagic.com/mailman/listinfo/phobos</a><br>
><br>
<br>
<br>
<br>
_______________________________________________<br>
phobos mailing list<br>
<a href="mailto:phobos@puremagic.com">phobos@puremagic.com</a><br>
<a href="http://lists.puremagic.com/mailman/listinfo/phobos" target="_blank">http://lists.puremagic.com/mailman/listinfo/phobos</a><br>
</div></div></blockquote></div><br>