[FFmpeg-cvslog] r16746 - trunk/libavcodec/rectangle.h

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> On Sat, Jan 24, 2009 at 03:30:36PM +0000, M?ns Rullg?rd wrote:
>> Diego Biurrun <diego at biurrun.de> writes:
>> 
>> > On Sat, Jan 24, 2009 at 02:51:22PM +0000, M?ns Rullg?rd wrote:
>> >> diego <subversion at mplayerhq.hu> writes:
>> >> 
>> >> > Log:
>> >> > Add required headers to fix 'make checkheaders'.
>> >> >
>> >> > --- trunk/libavcodec/rectangle.h	Sat Jan 24 15:32:22 2009	(r16745)
>> >> > +++ trunk/libavcodec/rectangle.h	Sat Jan 24 15:46:00 2009	(r16746)
>> >> > @@ -28,7 +28,10 @@
>> >> >  #ifndef AVCODEC_RECTANGLE_H
>> >> >  #define AVCODEC_RECTANGLE_H
>> >> >
>> >> > +#include <assert.h>
>> >> 
>> >> Be careful with adding assert.h.  You just enabled assert() checking
>> >> unconditionally in that file and anything that includes it.  Before
>> >> this commit, assert.h was included through internal.h, which sets
>> >> NDEBUG before including it.
>> >
>> > What is the proper solution then?  Adding #define NDEBUG or #including
>> > internal.h instead?
>> 
>> I don't know.  Michael refused to talk about it last time I brought it
>> up.
>
> huh?
> i suggested a solution and was entirely ignored by everyone
>
> The problem being that there are different types of asserts, some
> * being in speed critical code, these we dont want enabled unless realy needed
> * being about security related checks like (we know it fits in the buffer but
>   lets put an assert there anyway ...)
> * being neither of above
>
> if these 3 are split, we can handle things globally instead of per file.
> But currently
> Enabling everything globally means a huge speedloss and is unacceptable
> Disabling everything globally means debuging will be harder as users have
> them disabled and it might lead to secholes as well as missing bugs
> because the asserts that would give hints where disabled.

That means 3 different macros wrapping assert().

Examining all existing assert() calls (638 at the moment) and picking
the right version is a lot of work.  How about we add the required
macros now, and require new code to use the appropriate one.  That
will at least make sure the problem doesn't grow.

So what do we call the wrappers?  I suggest something like this:

- ff_assert_fast()     for speed-critical code
- ff_assert_critical() for security related checks
- ff_assert()          for everything else

For the non-critical ones, we need a controlling configure setting.

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