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