[phobos] I think we need to make an emergency release
David Simcha
dsimcha at gmail.com
Wed Aug 25 13:35:47 PDT 2010
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.
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.
On Wed, Aug 25, 2010 at 3:48 PM, Steve Schveighoffer <schveiguy at yahoo.com>wrote:
> 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
> >
>
>
>
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.puremagic.com/pipermail/phobos/attachments/20100825/92140323/attachment-0001.html>
More information about the phobos
mailing list