r25278 - in trunk: ffmpeg.c libavutil/Makefile libavutil/assert.h libavutil/avutil.h libavutil/rational.c
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 Added: trunk/libavutil/assert.h Modified: trunk/ffmpeg.c trunk/libavutil/Makefile trunk/libavutil/avutil.h trunk/libavutil/rational.c Modified: trunk/ffmpeg.c ============================================================================== --- trunk/ffmpeg.c Thu Sep 30 22:40:10 2010 (r25277) +++ trunk/ffmpeg.c Thu Sep 30 23:57:31 2010 (r25278) @@ -79,8 +79,7 @@ #include "cmdutils.h" -#undef NDEBUG -#include <assert.h> +#include "libavutil/assert.h" const char program_name[] = "FFmpeg"; const int program_birth_year = 2000; @@ -905,7 +904,7 @@ need_realloc: } }else if(audio_sync_method>1){ int comp= av_clip(delta, -audio_sync_method, audio_sync_method); - assert(ost->audio_resample); + av_assert0(ost->audio_resample); if(verbose > 2) fprintf(stderr, "compensating audio timestamp drift:%f compensation:%d in:%d\n", delta, comp, enc->sample_rate); // fprintf(stderr, "drift:%f len:%d opts:%"PRId64" ipts:%"PRId64" fifo:%d\n", delta, -1, ost->sync_opts, (int64_t)(get_sync_ipts(ost) * enc->sample_rate), av_fifo_size(ost->fifo)/(ost->st->codec->channels * 2)); @@ -1223,10 +1222,10 @@ static void do_video_out(AVFormatContext rightBand = ((int64_t)ist->st->codec->width * ost->original_rightBand / ost->original_width) & ~1; /* sanity check to ensure no bad band sizes sneak in */ - assert(topBand <= INT_MAX && topBand >= 0); - assert(bottomBand <= INT_MAX && bottomBand >= 0); - assert(leftBand <= INT_MAX && leftBand >= 0); - assert(rightBand <= INT_MAX && rightBand >= 0); + av_assert0(topBand <= INT_MAX && topBand >= 0); + av_assert0(bottomBand <= INT_MAX && bottomBand >= 0); + av_assert0(leftBand <= INT_MAX && leftBand >= 0); + av_assert0(rightBand <= INT_MAX && rightBand >= 0); ost->topBand = topBand; ost->bottomBand = bottomBand; @@ -1696,7 +1695,7 @@ static int output_packet(AVInputStream * //ost->sync_ipts = (double)(ist->pts + input_files_ts_offset[ist->file_index] - start_time)/ AV_TIME_BASE; if (ost->encoding_needed) { - assert(ist->decoding_needed); + av_assert0(ist->decoding_needed); switch(ost->st->codec->codec_type) { case AVMEDIA_TYPE_AUDIO: do_audio_out(os, ost, ist, decoded_data_buf, decoded_data_size); @@ -3628,7 +3627,7 @@ static void opt_new_stream(const char *o if (!strcmp(opt, "newvideo" )) new_video_stream (oc); else if (!strcmp(opt, "newaudio" )) new_audio_stream (oc); else if (!strcmp(opt, "newsubtitle")) new_subtitle_stream(oc); - else assert(0); + else av_assert0(0); } /* arg format is "output-stream-index:streamid-value". */ 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 \ Added: trunk/libavutil/assert.h ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ trunk/libavutil/assert.h Thu Sep 30 23:57:31 2010 (r25278) @@ -0,0 +1,59 @@ +/* + * copyright (c) 2010 Michael Niedermayer <michaelni at gmx.at> + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/** + * @file + * simple assert() macros that are a bit more flexible than ISO C assert(). + * @author Michael Niedermayer <michaelni at gmx.at> + */ + +#ifndef AVUTIL_ASSERT_H +#define AVUTIL_ASSERT_H + +#include "avutil.h" +#include "log.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) + + +/** + * assert() equivalent, that does not lie in speed critical code. + * 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) +#endif + + +/** + * assert() equivalent, that does lie in speed critical code. + */ +#if defined(ASSERT_LEVEL) && ASSERT_LEVEL > 1 +#define av_assert2(cond) av_assert_always(cond) +#else +#define av_assert2(cond) ((void)0) +#endif + +#endif Modified: trunk/libavutil/avutil.h ============================================================================== --- trunk/libavutil/avutil.h Thu Sep 30 22:40:10 2010 (r25277) +++ trunk/libavutil/avutil.h Thu Sep 30 23:57:31 2010 (r25278) @@ -40,7 +40,7 @@ #define AV_VERSION(a, b, c) AV_VERSION_DOT(a, b, c) #define LIBAVUTIL_VERSION_MAJOR 50 -#define LIBAVUTIL_VERSION_MINOR 31 +#define LIBAVUTIL_VERSION_MINOR 32 #define LIBAVUTIL_VERSION_MICRO 0 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ Modified: trunk/libavutil/rational.c ============================================================================== --- trunk/libavutil/rational.c Thu Sep 30 22:40:10 2010 (r25277) +++ trunk/libavutil/rational.c Thu Sep 30 23:57:31 2010 (r25278) @@ -25,7 +25,7 @@ * @author Michael Niedermayer <michaelni at gmx.at> */ -#include <assert.h> +#include "assert.h" //#include <math.h> #include <limits.h> @@ -67,7 +67,7 @@ int av_reduce(int *dst_num, int *dst_den num= den; den= next_den; } - assert(av_gcd(a1.num, a1.den) <= 1U); + av_assert2(av_gcd(a1.num, a1.den) <= 1U); *dst_num = sign ? -a1.num : a1.num; *dst_den = a1.den;
On Thu, Sep 30, 2010 at 11:57:31PM +0200, michael wrote:
+/** + * 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) + + +/** + * assert() equivalent, that does not lie in speed critical code. + * 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) +#endif
Huh? Did you mean av_assert0 instead of av_assert_always?
On Fri, Oct 01, 2010 at 12:01:40AM +0200, Reimar D?ffinger wrote:
On Thu, Sep 30, 2010 at 11:57:31PM +0200, michael wrote:
+/** + * 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) + + +/** + * assert() equivalent, that does not lie in speed critical code. + * 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) +#endif
Huh? Did you mean av_assert0 instead of av_assert_always?
yes, fixed [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I wish the Xiph folks would stop pretending they've got something they do not. Somehow I fear this will remain a wish. -- 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
Hi, 2010/9/30 M?ns Rullg?rd <mans at mansr.com>:
michael <subversion at mplayerhq.hu> writes:
-#include <assert.h> +#include "assert.h"
Using the same name as a standard header is practically guaranteed to cause confusion and mysterious bugs.
I second this, can you please rename this to avassert.h if nothing else? (Review would've been nice - what problem does this solve exactly?) Ronald
On Thu, Sep 30, 2010 at 07:37:36PM -0400, Ronald S. Bultje wrote:
Hi,
2010/9/30 M?ns Rullg?rd <mans at mansr.com>:
michael <subversion at mplayerhq.hu> writes:
-#include <assert.h> +#include "assert.h"
Using the same name as a standard header is practically guaranteed to cause confusion and mysterious bugs.
I second this, can you please rename this to avassert.h if nothing else?
as said, feel free to rename it, to me there is no ambiguity between #include <assert.h> and #include <libavutil/assert.h>
(Review would've been nice - what problem does this solve exactly?)
that of our code being filled with #undef NDEBUG hacks the problem of asserts() that are in speed critical code (aka enabling them slows things down significantly) and asserts that are in speed uncritical paths being inseperable. The thing is a devel wants to enable the ones that have no speed effect most of the time but theres a good reason not to enable the asserts that slow the code down This was discussed previously, no volunteers came forward to work on it. I knew mans would attack me when i commit no matter what i commit, i was a bit surprised to see your reply though. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
On date Friday 2010-10-01 03:30:16 +0200, Michael Niedermayer wrote:
On Thu, Sep 30, 2010 at 07:37:36PM -0400, Ronald S. Bultje wrote:
Hi,
2010/9/30 M?ns Rullg?rd <mans at mansr.com>:
michael <subversion at mplayerhq.hu> writes:
-#include <assert.h> +#include "assert.h"
Using the same name as a standard header is practically guaranteed to cause confusion and mysterious bugs.
I second this, can you please rename this to avassert.h if nothing else?
as said, feel free to rename it, to me there is no ambiguity between
#include <assert.h> and #include <libavutil/assert.h>
Note that we already have public headers which are named as standard headers, e.g. libavutil/error.h and <error.h>.
(Review would've been nice - what problem does this solve exactly?)
+1. Review for public API modifications should be *mandatory*, the maintainer has the last choice but at least he should give the opportunity for reviewing to the other developers. [...] Regards. -- FFmpeg = Fascinating and Fascinating Marvellous Prodigious Ecstatic Gospel
Stefano Sabatini <stefano.sabatini-lala at poste.it> writes:
On date Friday 2010-10-01 03:30:16 +0200, Michael Niedermayer wrote:
On Thu, Sep 30, 2010 at 07:37:36PM -0400, Ronald S. Bultje wrote:
Hi,
2010/9/30 M?ns Rullg?rd <mans at mansr.com>:
michael <subversion at mplayerhq.hu> writes:
-#include <assert.h> +#include "assert.h"
Using the same name as a standard header is practically guaranteed to cause confusion and mysterious bugs.
I second this, can you please rename this to avassert.h if nothing else?
as said, feel free to rename it, to me there is no ambiguity between
#include <assert.h> and #include <libavutil/assert.h>
Note that we already have public headers which are named as standard headers, e.g. libavutil/error.h and <error.h>.
There is no standard error.h. You were probably thinking of errno.h. -- M?ns Rullg?rd mans at mansr.com
Hi, On Sep 30, 2010, at 9:30 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Thu, Sep 30, 2010 at 07:37:36PM -0400, Ronald S. Bultje wrote:
(Review would've been nice - what problem does this solve exactly?)
that of our code being filled with #undef NDEBUG hacks
the problem of asserts() that are in speed critical code (aka enabling them slows things down significantly) and asserts that are in speed uncritical paths being inseperable. The thing is a devel wants to enable the ones that have no speed effect most of the time but theres a good reason not to enable the asserts that slow the code down
This was discussed previously, no volunteers came forward to work on it. I knew mans would attack me when i commit no matter what i commit, i was a bit surprised to see your reply though.
I'm not necessarily attacking... But I need to know how (and how not) to use this, or I'll keep complaining about already-solved problems. When do I use av_assert0/1/2()? What's the difference? How do I enable each separately or all together? I need a howto. ;-). Ronald
On Fri, Oct 01, 2010 at 07:58:23AM -0400, Ronald S. Bultje wrote:
Hi,
On Sep 30, 2010, at 9:30 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Thu, Sep 30, 2010 at 07:37:36PM -0400, Ronald S. Bultje wrote:
(Review would've been nice - what problem does this solve exactly?)
that of our code being filled with #undef NDEBUG hacks
the problem of asserts() that are in speed critical code (aka enabling them slows things down significantly) and asserts that are in speed uncritical paths being inseperable. The thing is a devel wants to enable the ones that have no speed effect most of the time but theres a good reason not to enable the asserts that slow the code down
This was discussed previously, no volunteers came forward to work on it. I knew mans would attack me when i commit no matter what i commit, i was a bit surprised to see your reply though.
I'm not necessarily attacking... But I need to know how (and how not) to use this, or I'll keep complaining about already-solved problems.
When do I use av_assert0/1/2()? What's the difference? How do I enable each separately or all together? I need a howto. ;-).
please explain me what is unclear and how i can clarify it av_assert0() is a assert check that is always enabled, its usefull for things the maintainer of a file want to be always checked. (like for example things that would lead to exploits or hard to debug problems would they fail) av_assert2() is intended for speed critical code, like for example checking that get/put_bits() dont go over the end of the allocated buffer which could lead to hard to reproduce crashes if it just goes a small amount over it av_assert1() lies between the 2, its for cases where the assert check does not cause slowdown but where it also does not appear to be overly important to the maintainer to always check it. i thought the documentation in assert.h was saying this clear enough. Suggestions to improve it are welcome To enable them, you have to wait until that part has been implemented, iam working on that [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Old school: Use the lowest level language in which you can solve the problem conveniently. New school: Use the highest level language in which the latest supercomputer can solve the problem without the user falling asleep waiting.
Michael Niedermayer <michaelni at gmx.at> writes:
On Thu, Sep 30, 2010 at 07:37:36PM -0400, Ronald S. Bultje wrote:
Hi,
2010/9/30 M?ns Rullg?rd <mans at mansr.com>:
michael <subversion at mplayerhq.hu> writes:
-#include <assert.h> +#include "assert.h"
Using the same name as a standard header is practically guaranteed to cause confusion and mysterious bugs.
I second this, can you please rename this to avassert.h if nothing else?
as said, feel free to rename it, to me there is no ambiguity between
#include <assert.h> and #include <libavutil/assert.h>
(Review would've been nice - what problem does this solve exactly?)
that of our code being filled with #undef NDEBUG hacks
the problem of asserts() that are in speed critical code (aka enabling them slows things down significantly) and asserts that are in speed uncritical paths being inseperable. The thing is a devel wants to enable the ones that have no speed effect most of the time but theres a good reason not to enable the asserts that slow the code down
This was discussed previously, no volunteers came forward to work on it.
I tried to do something like this a couple of years ago, and you flat-out rejected it. That put me off the idea.
I knew mans would attack me when i commit no matter what i commit,
Maybe you should try posting patches for a review and see what happens. A few nits aside, I like this change. It is the way you went about it I find revolting.
i was a bit surprised to see your reply though.
The dissent is stronger than you appear to realise. -- M?ns Rullg?rd mans at mansr.com
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 ..."
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
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
participants (6)
-
mans＠mansr.com -
michaelni＠gmx.at -
Reimar.Doeffinger＠gmx.de -
rsbultje＠gmail.com -
stefano.sabatini-lala＠poste.it -
subversion＠mplayerhq.hu