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

Thu Mar 13 04:25:16 CET 2014

On Thu, Mar 13, 2014 at 02:07:29AM +0100, Michael Niedermayer wrote:
> 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

or rather i can guess what it does from the previous discussion but
if i would just see this code that uses the macro i think i would be
confused
but thats sort of orthogonal to this patch

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

You can kill me, but you cannot change the truth.
-------------- 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/d806db7a/attachment.asc>