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

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> 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

You are not allowed to assume that.

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

That line was added by Reimar.  I merely moved it, just like git blame
would tell you.  Besides, one mistake by me or Reimar does not give
you a licence to make more of them.

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

I don't understand what it means.

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

Fair enough, although this isn't the standard assert().

>> > +#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

I still don't understand.

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

If it doesn't compile, it by definition does not work.  If you add
code with conditional branches, you should make a reasonable effort to
test both paths.  Clearly, you did not.

>> > -#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

The same reasons for naming avstring.h thus apply here as well.

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