[FFmpeg-cvslog] r19742 - trunk/libavutil/internal.h

[Reimar Döffinger]

On Sun, Aug 30, 2009 at 02:48:49AM +0200, Michael Niedermayer wrote:
> On Sat, Aug 29, 2009 at 11:44:29PM +0100, M?ns Rullg?rd wrote:
> > ramiro <subversion at mplayerhq.hu> writes:
> > 
> > > Author: ramiro
> > > Date: Sun Aug 30 00:38:48 2009
> > > New Revision: 19742
> > >
> > > Log:
> > > Add CHECKED_ALLOC macro.
> > > It works the same as CHECKED_ALLOCZ except that it does not zero the allocated
> > > memory.
> > >
> > > Modified:
> > >    trunk/libavutil/internal.h
> > >
> > > Modified: trunk/libavutil/internal.h
> > > ==============================================================================
> > > --- trunk/libavutil/internal.h	Sat Aug 29 23:04:18 2009	(r19741)
> > > +++ trunk/libavutil/internal.h	Sun Aug 30 00:38:48 2009	(r19742)
> > > @@ -249,6 +249,15 @@ if((y)<(x)){\
> > >  #define perror please_use_av_log_instead_of_perror
> > >  #endif
> > >
> > > +#define CHECKED_ALLOC(p, size)\
> > > +{\
> > > +    p= av_malloc(size);\
> > > +    if(p==NULL && (size)!=0){\
> > > +        av_log(NULL, AV_LOG_ERROR, "Cannot allocate memory.");\
> > > +        goto fail;\
> > > +    }\
> > > +}
> > 
> > Looks like I missed some discussions...  This should be wrapped in
> > do { } while(0) so if (foo) CHECKED_ALLOC(); else blah; can work.
> 
> if + else should always have a {} as it makes future patches simpler
> and costs no line of code.
> Besides this, i think using do/while for this is ugly.
> 
> I simply think the loss of readability trough do/while is a bigger issue
> than the gain for a case that seems rather unlikely to me ...

They do not even need an extra line, so personally I can't see where
there should even be a loss of readability, while even if it is rare I
can see someone spend ages debugging this when it is finally once used
wrong (note that this will not only be used in FFmpeg SVN but also by
people developing FFmpeg and not using the preferred style from the
start and certainly not knowing this discussion).

> Also to me, do/while is a loop and using it in macros like this feels
> very ugly to me ...

I can't help but say: just getting used to that use once seems like a
more reasonable solution than avoiding it.