Appender.reserve: Overzealous and wrong

[monarchdodra]

This contains two simple fixes:

== 1 ==
Appender.reserve is overzealous. Given a request of a capacity of size, it basically took the request, made that grow, and then reserved that. This results in things like:

auto app = Appender!(int[]);
app.reserve(65_000);
writeln(app.capacity); //Prints abous 107_000 on my machine. Pointless.

The fix is for to make ensureAddable's newCapacity to be based on the old capacity. Then, the final capacity is simply the max between the re-evaluated old=>new capacity, and the requested capacity.

== 2 ==
There was a little bug in reserve, where it called ensureAddable(requestedSize - currentCapacity) as opposed to ensureAddable(requestedSize - currentSize). This for example, things like this failed:

A case that failed:

Appender with 32_000 element, and 64_000 capacity.
Request 80_000 capacity
=> ensureAddable(48_000)
=> do nothing
=> resulting capacity is actually smaller than 80_000

http://d.puremagic.com/issues/show_bug.cgi?id=11335

[monarchdodra]

Pinging for review.

[DmitryOlshansky]

LGTM
With at least 2nd fix being an absolute nobrainer

P.S. One unrelated thing is that the algorithm for grow factor is not described at all leaving me wondering how did these equations poped up in the first place (sometime back in 2010 by Steve).
@schveiguy anything you can recall about it? :)

[monarchdodra]

I'm not sure what the origin of the algorithm is, but it makes sense to me. Using bsr is a "neat" way to provide logarithmic growth based on the currently amount of data already allocated. That said, having a known and documented growth scheme is better I think. Right now, it looks like "homebrew".

In any case, I didn't want to rewrite it. I also moved it outside of Appender, since the algorithm does not actually depend on T. I also slightly tweaked it to use unsigned algorithmic.

Rebased and ready for review.

[monarchdodra]

pining @blackwhale && @schveiguy .

[DmitryOlshansky]

I'd gladly merge but I'm not a commiter... And Steven seems to be quite busy elsewhere.
In other words we've got to ping somebody else to give it a glance .. @AndrejMitrovic @9rnsr

[monarchdodra]

pinging @schveiguy , @AndrejMitrovic and @9rnsr .

[ghost]

LGTM.

[ghost]

Thanks! (but please make bugzilla reports for these pulls in the future)

[schveiguy]

I know this is really really old, but somehow I happened to notice it in my email I was about to delete.

The newCapacity function was cut and paste from the runtime's append growth function. I did NOT write it, but I did fix it a long time ago. Somewhere in the history of d runtime mailing list, you can find what I had said. There were some bizarre relationships between size of the element and the growth function.

In any case, I agree if you are simply reserving, the appender does not need to use new capacity at all. It should only be in the case of appending. In the case of reserve, it should just reserve exactly that much, plus whatever is required to make a complete block. I think that's the way arr.reserve works also.

So good change, thanks!