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

[Michael Niedermayer]

On Wed, Mar 12, 2014 at 03:34:41PM +0100, Lukasz M wrote:
> On 12 March 2014 15:21, Lukasz M <lukasz.m.luki at gmail.com> wrote:
> 
> > On 9 March 2014 17:04, Michael Niedermayer <michaelni at gmx.at> wrote:
> >
> >> On Thu, Mar 06, 2014 at 11:03:09PM +0100, Lukasz Marek wrote:
> >> > On 06.03.2014 03:06, Michael Niedermayer wrote:
> >> > >On Wed, Mar 05, 2014 at 07:34:35PM +0100, Nicolas George wrote:
> >> > >>Le quintidi 15 ventôse, an CCXXII, wm4 a écrit :
> >> > >>>Unfortunately, this is pretty horrible. GET_UTF8 style.
> >> > >>
> >> > >>That is your personal taste, it is not corroborated by any argument.
> >> > >
> >> > >compiler errors to such macros are cryptic and cost more time
> >> > >to fix. get the "code passed as argument" wrong and the compiler
> >> > >will likely have a hard time to figure out what is wrong, it really
> >> > >cant know if your macro or the code using it is at fault if the
> >> > >result is a syntax error
> >> > >This doesnt occur with functions
> >> > >
> >> > >
> >> > >
> >> > >>
> >> > >>And the beauty of that kind of API is: if you don't like it, don't
> >> use it.
> >> > >>
> >> > >>>It could be simplified a bit. First off, you don't need to put the
> >> code
> >> > >>>for "success" as macro parameter. You could call the macro something
> >> > >>>like AV_DYNARRAY_GROW, and put the "success" code after it.
> >> > >>
> >> > >>That means the failure case has to be some kind a jump, or it needs
> >> an extra
> >> > >>condition. That seems to make the macro less usable for no apparent
> >> reason
> >> > >>except alleged "simplification".
> >> > >>
> >> > >>>Second, you could drop the type for the size. You'll only ever use
> >> > >>>size_t or int as index, so it's better to duplicate the macro for
> >> those
> >> > >>>two types, instead of wasting 2 macro parameters on them.
> >> > >>
> >> > >>Until someone is forced by some reason or another to use a variable of
> >> > >>another type, for example to interact with a library that uses long.
> >> > >>
> >> > >
> >> > >>It is this kind of lack of foresight that leads to having two
> >> > >>implementations where there could be just one for the start.
> >> > >
> >> > >An API thats designed with
> >> > >"And the beauty of that kind of API is: if you don't like it, don't
> >> use it."
> >> > >will probably not improve that
> >> > >
> >> > >Its important that the developers like the API because if they end
> >> > >up not using it then no matter how great it is in theory it wont
> >> > >help them in practice
> >> > >
> >> > >
> >> > >>
> >> > >>Also, a type parameter does not cost anything in a macro, there is
> >> just no
> >> > >>reason to omit it.
> >> > >
> >> > >between
> >> > >
> >> > >printf(future extension, we might use this, 10234, 9999, 1234, yes,
> >> > >        no, reserved, hello_world_string, goto error /* this cant fail
> >> */);
> >> > >
> >> > >and
> >> > >
> >> > >printf(hello_world_string);
> >> > >
> >> > >i find the later more readable, that is needing less of my time.
> >> >
> >> > So, to summarize, should I fix last commit (change return error
> >> > code) or replace it with macro?
> >>
> >> you can fix the error code, or wait for nicolas to implement the
> >> function based on his macro i guess
> >> i have no real preferrance
> >>
> >
> > Implemented using Nicolas' macro
> >
> 
> Forgot to squash small fix, one more patch

>  mem.c |   13 +++++++++++++
>  mem.h |   19 +++++++++++++++++--
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 7fad1be9083fcaef4435fa0273f79bceca98821b  0001-lavu-mem-add-av_dynarray_add_nofree-function.patch
> From 4cadb3328ba018b37c5dfe05a637b50a262151c6 Mon Sep 17 00:00:00 2001
> From: Lukasz Marek <lukasz.m.luki at gmail.com>
> Date: Tue, 25 Feb 2014 01:06:06 +0100
> Subject: [PATCH] lavu/mem: add av_dynarray_add_nofree function
> 
> av_dynarray_add_nofree function have similar functionality
> as existing av_dynarray_add, but it doesn't deallocate memory
> on fails.
> 
> TODO: minor bump and update doc/APIChanges
> 
> Signed-off-by: Lukasz Marek <lukasz.m.luki at gmail.com>
> ---
>  libavutil/mem.c |   13 +++++++++++++
>  libavutil/mem.h |   19 +++++++++++++++++--
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/libavutil/mem.c b/libavutil/mem.c
> index 7206ddc..b9b5742 100644
> --- a/libavutil/mem.c
> +++ b/libavutil/mem.c
> @@ -278,6 +278,19 @@ void *av_memdup(const void *p, size_t size)
>      return ptr;
>  }
>  
> +int av_dynarray_add_nofree(void *tab_ptr, int *nb_ptr, void *elem)
> +{

> +    intptr_t *tab = *(intptr_t**)tab_ptr;

undefined behavior (strict aliasing violation)


> +
> +    AV_DYNARRAY_ADD(INT_MAX, sizeof(*tab), tab, *nb_ptr, {
> +        tab[*nb_ptr] = (intptr_t)elem;
> +        *(intptr_t **)tab_ptr = tab;
> +    }, {
> +        return AVERROR(ENOMEM);
> +    });

cant comment, i have no clue what this does

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140313/20672e66/attachment.asc>