[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:57:22PM +0100, M?ns Rullg?rd wrote:
> 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.

iam, thats why stdlib.h and other standard headers are included there
or to say it the other way around, its to not have to duplicate dozen includes
in every file.


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

no doubt that the line is not ideally formated and i will fix this but your
agression is completely out of place.
As you said you moved this code around and saw no need to reformat it just
now when i commit you come and attack.


[...]
> >> > -#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.

as ive said, i dont mind you renaming it
but until you provide concrete reasons for such change there is no problem
that needs me to fix it. At this point its a bikeshed and i have no time for
that

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

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- 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/cde470f6/attachment-0001.pgp>