[FFmpeg-devel] Moving if(constant expression) to preprocessor?

[Måns Rullgård]

"Axel Holzinger" <aholzinger at gmx.de> writes:

> M?ns Rullg?rd wrote:
>> Sent: Sunday, September 19, 2010 7:00 PM
>> To: FFmpeg development discussions and patches
>> Subject: Re: [FFmpeg-devel] Moving if(constant expression) to 
>> preprocessor?
>> 
>> "Axel Holzinger" <aholzinger at gmx.de> writes:
>> 
>> > M?ns Rullg?rd and nice are mutually exclusive. Does this help?
>> 
>> Please refrain from ad hominem attacks.
>
> That was just a replique on your attack on boost, so forget it.

I never forget.

>> > Here is the code that works and that's tested with gcc:
>> >
>> > #define AV_COND_IIF(bit, t, f) AV_COND_IIF_I(bit, t, f) #define 
>> > AV_COND_IIF_I(bit, t, f) AV_COND_IIF_II(AV_COND_IIF_ ## bit(t, f))
>
>> > #define AV_COND_IIF_II(id) id #define AV_COND_IIF_0(t, f) f
>> > #define  AV_COND_IIF_1(t, f) t #define AV_COND_BOOL(x) AV_COND_BOOL_I(x) 
>> > #define AV_COND_BOOL_I(x) AV_COND_BOOL_ ## x #define AV_COND_BOOL_0 0 
>> > #define AV_COND_BOOL_1 1 #define AV_COND_IF(cond, t, f) 
>> > AV_COND_IIF(AV_COND_BOOL(cond), t, f)
>> 
>> That is quite a bit more obfuscated than it needs to be.  The 
>> following actually works:
>> 
>> $ cat foo.c
>> #define AV_IF_0(t, f) f
>> #define AV_IF_1(t, f) t
>> #define AV_IF(c, t, f)  AV_IF2(c, t, f)
>> #define AV_IF2(c, t, f) AV_IF_##c(t, f)
>
> Smaller and simpler, perfect. I'm fine with that and works for me.

Whether or not you are "fine" with anything is irrelevant as far as we
are concerned.

>> > Usage:
>> > Before:
>> > #define REGISTER_HWACCEL(X,x) { \
>> >           extern AVHWAccel x##_hwaccel; \
>> >           if(CONFIG_##X##_HWACCEL) 
>> av_register_hwaccel(&x##_hwaccel);
>> > }
>> >
>> > With the macro:
>> > #define REGISTER_HWACCEL(X,x) { \
>> >           extern AVHWAccel x##_hwaccel; \
>> >           AV_COND_IF(CONFIG_##X##_HWACCEL,
>> > (av_register_hwaccel(&x##_hwaccel)),) ; }
>> >
>> >> However, it is far more convoluted than the current code
>> >
>> > What?
>> 
>> 10 obfuscated macros are more convoluted than none.
>
> There are a lot more multiline obfuscated macros already in FFmpeg.

They all provide some advantage over not being there.

> The singleline ones aren't that complicated.

A stack of one-line macros is often much harder to understand than a
single, large one.

> And you even got it more simple. So go ahead and use them.

You will not tell me what to do.

>> >> for no gain,
>> >
>> > It makes it possible to completely disable optimisations.
>> 
>> I do not consider that a gain.
>
> Isn't having the oportunity to have the choice a gain?

A choice is only a gain if the alternatives provide mutually exclusive
advantages.

>> > To date the configure script offers to disable optimisations, but 
>> > doesn't do so.  One could call this a bug, at least it relies on 
>> > compiler specifics that are not mandated by C99.
>> 
>> We rely on a lot of things outside the scope of C99, for 
>> example linkers.
>
> Come on, that's silly. Relying on dead code elimination is really
> obfuscating, because you can't understand from reading the code that
> this only works with optimisation. Having a conditional preprocessor
> if makes it much more clear, that the code will not be compiled, if
> the condition is not met.

Dead code elimination is an implementation detail.  The high-level
logic is quite clear.

>> >> and Reimar's comments are valid too.
>> >
>> > I think Reimar's comments are beside the point. The only thing the
>> > compiler does, if a runtime if is used instead of a macro, 
>> > is that it checks the syntax of the call, nothing else.
>> 
>> I consider it a good thing having the syntax checked.
>
> It's far more probable that a potential issue is inside a function
> call than in the function call itself, whereas most of the function
> calls we're talking about (the ones in the register macros) don't have
> parameters at all. So I still tend to think that this not really an
> argument and is beside the point.

Having some checks is still better than having none.

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