[FFmpeg-devel] Proposal on clearly delineating nonC99 code in FFmpeg

[Måns Rullgård]

Roman Shaposhnik <rvs at sun.com> writes:

> Guys,
>
> this item has been on my TODO list for quite sometime and it has
> to do with the constructs like this one (from libavutil/mem.h):
>
> ------------------------------------------------------------------------
> #ifdef __GNUC__
>   #define DECLARE_ALIGNED(n,t,v)       t v __attribute__ ((aligned (n)))
> #else
>   #define DECLARE_ALIGNED(n,t,v)      __declspec(align(n)) t v
> #endif
> -------------------------------------------------------------------------
>
> The problem I have with the above is that we're not testing for the
> right thing here. The fact that GCC is being used is irrelevant, since
> all we really care about is whether compiler supports 'aligned'
> attribute. But that's just the first layer this particular onion has.
> The second one is the fact that whether GCC-style aligned attributes are
> supported or not is also irrelevant. All we're looking for is ANY kind
> of C99 language extension that would allow us to declare aligned 
> variables (which by the way makes this particular case very broken for
> any compiler that doesn't claim to be __GNUC__ but also doesn't happen
> to be Microsoft Visual Studio).

Totally agree.

> So, in order to address the second issue I propose that we create 
> a dedicated header file: ffmpeg/c99_extensions.h where all things
> like DECLARE_ALIGNED would be declared. 

It is fairly common to call such headers compiler.h, so perhaps we
should follow suit.

> In order to address the first issue I propose that we don't test for
> particular compilers even within the ffmpeg/c99_extensions.h but
> we delegate the testing to ./configure script. So that ANY compiler
> that supports, lets say, GCC-style attributes would be treated as
> a first class citizen without resorting to bogus pre-defines of
> __GNUC__

Yes, that's how it should be done.  Patches are, as always, welcome ;-)

> I see this proposal giving us two crucial benefits:
>    * a centralized location for all things that break out of
>      C99 realm would make the job of anybody porting FFmpeg to 
>      different platforms much easier. Of course, every construct
>      we put in this header file will be heavily commented explaining
>      what we expect it to do and giving hints on how it can be
>      ported to C99 platforms without direct support for it.
>
>    * testing whether a known feature actually works instead of
>      assuming that it always does if compiler is X and it always
>      doesn't if compiler happens to be Y is a much cleaner 
>      approach for both cases (in fact I've seen GCC break some
>      of the lesser used attributes from time to time -- something
>      that won't get caught unless you actually test for the feature).

Crafting a test that really determines whether something works is
often less than trivial.  What appears to work in a simple test case
may well fail in obscure ways when used in real code.  This
notwithstanding, testing whether the constructs in question compile OK
is certainly better than only testing a preprocessor definition.

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