[FFmpeg-cvslog] r25278 - in trunk: ffmpeg.c libavutil/Makefile libavutil/assert.h libavutil/avutil.h libavutil/rational.c

[Michael Niedermayer]

On Fri, Oct 01, 2010 at 12:33:24AM +0100, M?ns Rullg?rd wrote:
> michael <subversion at mplayerhq.hu> writes:
> 
> > Author: michael
> > Date: Thu Sep 30 23:57:31 2010
> > New Revision: 25278
> >
> > Log:
> > av_assert() system.
> > With this the developer can now choose if he wants an assert always enabled or at which
> > compile time assert level. This can thus replace the #define NDEBUG hacks
> >
> > Modified: trunk/libavutil/Makefile
> > ==============================================================================
> > --- trunk/libavutil/Makefile	Thu Sep 30 22:40:10 2010	(r25277)
> > +++ trunk/libavutil/Makefile	Thu Sep 30 23:57:31 2010	(r25278)
> > @@ -3,6 +3,7 @@ include $(SUBDIR)../config.mak
> >  NAME = avutil
> >
> >  HEADERS = adler32.h                                                     \
> > +          assert.h                                                      \
> >            attributes.h                                                  \
> >            avstring.h                                                    \
> >            avutil.h                                                      \
> 
> I would much appreciate if you posted public headers (at the very
> least) for review before committing.  You require this of others, so
> please abide by the rule yourself.
>
> > +#ifndef AVUTIL_ASSERT_H
> > +#define AVUTIL_ASSERT_H
> > +
> > +#include "avutil.h"
> > +#include "log.h"
> 
> Missing #include <stdlib.h>

no, its included by common.h


> 
> > +/**
> > + * assert() equivalent, that is always enabled.
> > + */
> > +#define av_assert0(cond) do {if(!(cond)) { av_log(NULL, AV_LOG_FATAL, "Assertion %s failed at %s:%d\n", AV_STRINGIFY(cond), __FILE__, __LINE__); abort(); }}while(0)
> 
> Please format this ridiculously long line in a sane way.

right after you format yours on which this is based in a sane way:
7046        mru #define av_abort()      do { av_log(NULL, AV_LOG_ERROR, "Abort at %s:%d\n", __FILE__, __LINE__); abort(); } while (0)


> 
> > +/**
> > + * assert() equivalent, that does not lie in speed critical code.
> 
> What is that supposed to mean?

that what is written there, iam sure it can be improved though


> 
> > + * These asserts() thus can be enabled without fearing speedloss.
> > + */
> > +#if defined(ASSERT_LEVEL) && ASSERT_LEVEL > 0
> > +#define av_assert1(cond) av_assert_always(cond)
> > +#else
> > +#define av_assert1(cond) ((void)0)
> 
> WTF?

the C spec mandates assert() to be defined to ((void)0) when it is disabled


> 
> > +#endif
> > +
> > +
> > +/**
> > + * assert() equivalent, that does lie in speed critical code.
> 
> What is that supposed to mean?

that what is written there, iam sure it can be improved though


> 
> > + */
> > +#if defined(ASSERT_LEVEL) && ASSERT_LEVEL > 1
> > +#define av_assert2(cond) av_assert_always(cond)
> > +#else
> > +#define av_assert2(cond) ((void)0)
> > +#endif
> 
> Maybe more meaningful names for these macros would be a good idea.
> You couldn't even get them right yourself in this very commit.  You
> also obviously did not test it.  I find that sloppy to the point of
> being unacceptable.  Doubly so for someone who already considers
> himself exempt from the usual review process.

The code was tested and works correctly, some code pathes are work in progress
and arent compiled yet, blowing a typo in #if 0 code out of proportion is
quite annoying, id appreciate if you could do something usefull instead.


> 
> > -#include <assert.h>
> > +#include "assert.h"
> 
> Using the same name as a standard header is practically guaranteed to
> cause confusion and mysterious bugs.

feel free to change the name, though i must point out that its
libavutil/assert.h
in user applications and in all libs except libavutil so the potential
for confusion seems quite small

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 1
"Used only once"    - "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20101001/282e067f/attachment.pgp>