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.

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
> 

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

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
> 

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
> 

BTW, this new code does not crash on the original code in bug 4681, and Don's code in comment 8.

-Steve



----- 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 3:48:22 PM
> Subject: Re: [phobos] I think we need to make an emergency release
> 
> 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
> 

On Wednesday 25 August 2010 12:48:22 Steve Schveighoffer 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.

I've never liked the fact that appender worked that way, and that's part of the reason that I haven't used it much. I'm all for a safer implementation. Of course, because I haven't use appender much, my take on things could be quite a bit different from those who have (and they're the ones who are going to be affected most by this).

- Jonathan M Davis

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>

On 8/25/10 6:50 PDT, Steve Schveighoffer wrote:
> OK, looked at it for a while, I wasn't clear originally on what Appender is exactly supposed to do.

The idea is to provide a reference type that efficiently appends to an array. Previously Appended had two fields (a pointer to the array and the capacity) so it wasn't a reference type. This made it impossible to pass Appender objects by value and have it work properly.

> 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.

Great, I'm seeing your other email now.


Andrei

On 8/25/10 12:48 PDT, Steve Schveighoffer wrote:
> 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).

I don't think this is good. When you format stuff you do want to append the formatted result incrementally to the string.

> 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;

I think this looks good - why not make arr of type pointer to array and support append to existing arrays?

> /**
> 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);

Appender was partly created to avoid the issue the comment warns about.


Andrei