[FFmpeg-devel] [PATCH] split off h264_mb.h from h264.h

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> On Wed, Apr 14, 2010 at 02:19:10AM +0100, M?ns Rullg?rd wrote:
>> Diego Biurrun <diego at biurrun.de> writes:
>> 
>> > On Wed, Apr 14, 2010 at 02:42:24AM +0200, Michael Niedermayer wrote:
>> >> On Wed, Apr 14, 2010 at 01:05:40AM +0200, Diego Biurrun wrote:
>> >> > This patch splits off a separate header file for decode_mb_skip() and
>> >> > friends from h264.h.  I consider it a sensible idea in general and it
>> >> > eliminates more of those pesky 'defined but not used' warnings.
>> >> > 
>> >> > Since this just moves static functions like my previous patch I assume
>> >> > it is similarly safe.  OK to apply?
>> >> > 
>> >> >  h264.h       |  541 -------------------------------------------
>> >> >  h264_cabac.c |    1 
>> >> >  h264_cavlc.c |    1 
>> >> >  h264_mb.h    |  735 -----------------------------------------------------------
>> >> 
>> >> iam against this patch. The placement of code into files becomes random
>> >> with it and merely guided by avoidance of silly warnings
>> >> these warnings should be disabled!
>> 
>> Over my dead body.
>
> I can mark the functions as av_unused if you prefer that

No, that will not make it any better.  Why do you place huge functions
in a header file?  If you intend for them to be inlined, they should
be marked as such.  If not, they belong in a C file with only a
prototype in the header.

>> > I beg to differ.  h264.h is huge at 1300 lines of code and it's just a
>> > grab bag of many things H.264-related with no particular order or
>> > structure.  Splitting the macroblock code off from it is not making
>> > things more random and is not just guided by warning avoidance.
>> 
>> In this case, the warnings are working exactly as intended.  Unused
>> functions being visible to the compiler are a sign of bad structure.
>
> the patch doesnt improve the structure.
> And we have hundreads of unused static inline functions in headers, and

Yes, *inline*.  These are small functions meant to be inlined.  There
is a reason gcc doesn't warn about unused inline functions.

> any solution to that would make the code worse.
> split headers based on 1 function per header or #ifdef around every function

Now you're just being deliberately obtuse again.  Please stop.

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