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

[Måns Rullgård]

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>

> +/**
> + * 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.

> +/**
> + * assert() equivalent, that does not lie in speed critical code.

What is that supposed to mean?

> + * 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?

> +#endif
> +
> +
> +/**
> + * assert() equivalent, that does lie in speed critical code.

What is that supposed to mean?

> + */
> +#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.

> -#include <assert.h>
> +#include "assert.h"

Using the same name as a standard header is practically guaranteed to
cause confusion and mysterious bugs.

-- 
M?ns Rullg?rd
mans at mansr.com