D array expansion and non-deterministic re-allocation

[Bartosz Milewski]

Steven Schveighoffer Wrote:
> > Imagine you're reviewing this code written by a relative newbee:
> >
> > char[] quote(char[] word) {
> >   word.length += 2;
> >   word[length-1] = '"';
> >   for(int i = word.length-2; i > 0; --i)
> >     word[i] = word[i - 1];
> >   word[0] = '"';
> >   return word;
> > }
> >
> > void storeFoos(char[] buffer, string pattern, Container c)
> > {
> > 	char[] res = find(buffer, pattern);
> > 	c.store(quote(res[0..pattern.len]);
> > }
> >
> > Is this buggy? If so, how and when will the bug manifest itself?
> 
> In my mind it is not buggy.  If quote or storeFoos is not meant to be able  
> to molest the data, it should require a const(char)[].  If this was D1  
> code, I'd say it depends on the purpose of storeFoos, if storeFoos is  
> expecting to own the input buffer, it's not buggy.  If it's not, then  
> storeFoos is buggy (in all cases).  But we aren't talking about D1.
 
Yes, a lot depends on what's expected of storeFoos. Suppose that the surrounding code expects the buffer not to be modified. The newbee didn't want to idup the buffer because in most buffers the pattern is not found.

> > Or another story. An enthusiastic programmer comes to you with a very  
> > clever rewrite of a function. This is the original:
> >
> > T[] duplicate(T)(T[] src) {
> >    static if (is(T == invariant))
> >       return src.idup;
> >    else
> >       return src.dup;
> > }
> >
> > This is his improved version:
> >
> > T[] duplicate(T)(T[] src) {
> >    T tmp = src[$-1];
> >    src.length -= 1;
> >    src ~= tmp;
> >    return src;
> > }
> >
> > No need for static if! The extension is bound to re-allocate because of  
> > stomping rules. Will this always work, or is there a gotcha?
> 
> He forgot to check for length 0.
> 

That he did. So now he goes back to the drawing board and adds an early return for length 0. Is his code correct now?