[vlc-devel] [RFC 3/3] vlc_arrays: append items in amortized O(1)

[Filip Roséen]

Hi,

On 2018-07-20 11:24, Romain Vimont wrote:

> Thank you for your review.

I am too tired to write code so no worries, in either case; you are
welcome!

> > I assume you want to shrink *if* `array->i_capacity` is more than 1.5
> > times the current size, not if it is less than that.
> 
> This is correct: if array->i_capacity is more than 1.5 times the current
> size, then the if-condition is false, so we don't return immediately...
> so we shrink.

This alone is proof that I should go and sleep, so I will do that
straight after sending this email.

> I probably should make this code more clear, it looks confusing.

Might just be me who has been awake for too long, though if you can
make it more readable it is far from a bad thing - thanks.

> > Your tests should also assert on the return-code for functions which
> > may fail.
> 
> I could, but I didn't do it because their failure would be triggered by
> the following asserts anyway (e.g. if append() fails, count() will be
> incorrect). And it would just add explicit assertions about
> out-of-memory.

We have other tests using `asserts` in some places, and tests that
even `assert` that memory-allocation succeeds. The reason is precisely
as you pointed out: if *append* fails, *count* will be flagged as the
culprit.

Even though you might know that the function will only fail due to a
memory allocation, who knows what changes to the function will come in
the future (if any)? Better safe than sorry, imo.

> > There must be a patch missing in this set, perhaps it is intentional,
> > but as we currently have no tests for arrays in `src/test/array.c` the
> > above is very odd.
> 
> Yes, I noted in the cover-letter:

I should probably start reading things more carefully, instead of
jumping straight to the implementation; sorry, my bad.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20180720/b1939133/attachment.html>