[FFmpeg-devel] C99 or GCC extensions

[Måns Rullgård]

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

> Hi again,
>
> Axel Holzinger <aholzinger at gmx.de> writes:
>> "M?ns Rullg?rd" <mans at mansr.com> writes:
>> 
>> > ...
>> >
>> > It is almost certainly remnants of days gone by, when rules
>> were
>> > less strict.
>> >
>> > > If not, what is the current policy on this and are patches
>> > > welcome to remove GCC specifics in favour of C99?
>> >
>> > The current policy is that new code should compile with a C99
>> > compiler, unless it is an optional assembler optimisation.
>> > Patches to rectify non-standard code are always welcome.
>> 
>> Great, lavori in corso.
>
> I looked a bit at statements in expressions, which are not C99
> AFAIK and I found an issue with the FASTDIV implementation in
> internal.h of libavutil.
>
> The FASTDIV macro uses statements in an expression. The only way
> to come around this I can see with C99 is to move the code in an
> inlined function.

I see no harm in changing these constructs into inline functions.

> To enter the discussion on how to fix this correctly I made a
> patch which implements a function named FASTDIV, which is not
> entirely political correct as capital names normally shouldn't
> be used for functions. OTOH I didn't want to completely change
> the structure.
>
> It would also be possible to always implement the function and
> inside #ifdef the different architectures. I don't hold stock
> options in any of the possibilities, I'm just interested in
> C99 "conformizing" the code :-)

I'm all for making the code standards-compliant.

> Index: internal.h
> ===================================================================
> --- internal.h	(revision 14169)
> +++ internal.h	(working copy)
> @@ -144,27 +144,27 @@
>  extern const uint32_t ff_inverse[256];
>  
>  #if defined(ARCH_X86)
> -#    define FASTDIV(a,b) \
> -    ({\
> -        int ret,dmy;\
> -        asm volatile(\
> -            "mull %3"\
> -            :"=d"(ret),"=a"(dmy)\
> -            :"1"(a),"g"(ff_inverse[b])\
> -            );\
> -        ret;\
> -    })
> +uint32_t static av_always_inline FASTDIV(uint32_t a, uint32_t b)

It is customary to write "static type" rather than "type static".

> +{
> +    uint32_t ret, dmy;
> +    asm volatile(\
> +        "mull %3"\
> +        :"=d"(ret),"=a"(dmy)\
> +        :"1"(a),"g"(ff_inverse[b])\
> +        );\
> +    return ret;
> +}

Lose the backslashes.

>  #elif defined(ARCH_ARMV4L)
> -#    define FASTDIV(a,b) \
> -    ({\
> -        int ret,dmy;\
> -        asm volatile(\
> -            "umull %1, %0, %2, %3"\
> -            :"=&r"(ret),"=&r"(dmy)\
> -            :"r"(a),"r"(ff_inverse[b])\
> -            );\
> -        ret;\
> -    })
> +uint32_t static av_always_inline FASTDIV(uint32_t a, uint32_t b)
> +{
> +    uint32_t ret, dmy;
> +    asm volatile(\
> +        "umull %1, %0, %2, %3"\
> +        :"=&r"(ret),"=&r"(dmy)\
> +        :"r"(a),"r"(ff_inverse[b])\
> +        );\
> +    return ret;
> +}

Ditto.

>  #elif defined(CONFIG_FASTDIV)
>  #    define FASTDIV(a,b)   ((uint32_t)((((uint64_t)a)*ff_inverse[b])>>32))
>  #else

Unrelated to this change, machine-specific code should go into
subdirs, IMHO.

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