[FFmpeg-devel] [PATCH 1/4] lavu: add simple array implementation

[Ronald S. Bultje]

Hi,

On Fri, Feb 28, 2014 at 8:54 PM, Lukasz Marek <lukasz.m.luki at gmail.com>wrote:

> +/**
>>> + * callback called inside av_array_free() to free each element of the
>>> array.
>>> + */
>>> +typedef void (*avarray_element_deleter)(void *element);
>>>
>>
>> A type name in all lowercase seems rather unusual in the av* API.
>>
>
> I don't understand. This is a function. I have already added such
> declarations and no remarks were posted.
>
>
>
>  On the whole, I am very dubious with this API.
>>
>> First, there are already several parts of array implementations in the
>> code,
>> each with various pros and cons. Adding a new one will only make matter
>> worse. See av_dynarray_add() and av_dynarray2_add(): they both are
>> interesting, but IIRC they have flaw that prevent them to be used in a
>> generic case. Fixing them seems like a better idea.
>>
>
> The issue with mentioned functions is they delete buffer and doesn't
> bother to delete nested pointers. In fact they only reallocate memory when
> need and append new element. I updated previous patch according your
> suggestions and I hope you consider it as fixed version of existing API
>
>
>  Second, I do not like the insert/append/prepend stuff, and the
>> av_array_at()
>> function even worse. To set the value of an array cell, you write tab[42]
>> =
>> 1729.
>>
>
> I have problems to agree with it as a whole. I can understand that direct
> access to buffer may be useful, but this approach leaves implementation of
> everything on the developer each time they need an array. So what developer
> may push element to the array by tab[i] = X, but they need to check if
> buffer is full and reallocate it. Also, it works only when elements doesn't
> contains dynamically allocated pointers or new element is push to the end.
>
>
> The insert/append/prepend/at stuff is the kind of thing you find in
>
>> the ultra-generic abstract java classes. So really, you would just need a
>> single function that ensures a cell is free.
>>
>
> There is nothing wrong in being generic, glib also provides an array
> implementation and it is not a Java...


You're adding it to lavu, and it's not marked as internal/experimental. If
it stinks or needs 3 API updates or re-implementations, the old cruft
sticks around for years to come.

That's why we now have 2 (and with yours, soon 3) array implementations.
Can we stick to one? At least java uses the same API for all its array/list
things.

Ronald