[FFmpeg-devel] [PATCH] remove useless mm.c close function

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> On Sun, Aug 24, 2008 at 03:28:21PM +0100, M?ns Rullg?rd wrote:
>> Michael Niedermayer <michaelni at gmx.at> writes:
>> 
>> > On Sun, Aug 24, 2008 at 03:33:36PM +0200, Aurelien Jacobs wrote:
>> >> Reimar D?ffinger wrote:
>> >> 
>> >> > Hello,
>> >> > admittedly not tested, but seems quite obvious.
>> >> 
>> >> That indeed looks safe.
>> >> I guess you can commit such trivial patches without asking.
>> >> 
>> >> > Unrelated, but wasn't there a macro to do the standard
>> >> > sizeof(arr)/sizeof(*arr) to get the number of array
>> >> > elements? I've seen quite a few code places that could
>> >> > use it - either to de-hardcode assumptions or to de-uglify..
>> >> 
>> >> I proposed a patch a patch adding such a macro some times
>> >> ago [1]. But Michael was not fond of it.
>> >> I still think it is a good idea. Maybe it's time to push
>> >> it again ?
>> >> My original patch (attached for reference) added two macro.
>> >> One for 1D array and one for 2D array. The one for 2D
>> >> array had a quite bad name and wasn't much used.
>> >> So for now I propose only adding this macro:
>> >> 
>> >> #define FF_ARRAY_SIZE(a)  (sizeof(a)/sizeof(*a))
>> >> 
>> >> Michael ? Are you still against this ? Strongly, or just a
>> >> little bit ? ;-)
>> >
>> > I have no strong oppinion on this specific case but overall there are
>> > IMHO FAR too many macros in ffmpeg already, and we should try to reduce
>> > their number not increase it.
>> > Thus if this is added then id like to see at least 1 other macro removed
>> > and
>> 
>> What's suddenly so bad about macros?
>
> to understand:
> for(i=0; i < sizeof(a)/sizeof(*a); i++){ ...
> you just need to know C
>
> to understand
> for(i=0; i < FF_ARRAY_SIZE(a); i++){ ...
> you need to know what FF_ARRAY_SIZE does as well.
>
> The more such helper functions and macros ffmpeg has and uses, the
> more difficult it is for a person "outside" to understand and work
> with the code.

IMHO, it's more important that the code is easy to work with for
people on the inside than for those on the outside.

> Thus when adding new ones we should really consider
> * how easy to understand is it when some average to good C programmer for
>   the first time sees it?

Someone who can't look up a simple macro definition has no business
working on FFmpeg.  We don't want average programmers.  We want only
the best, right?

> * Does he have to look up the definiton or is the name alone enough?

Good naming is of course always important.

> * what effect does it have on readability for experienced ffmpeg developers?

Presumably whoever proposes the addition finds it more readable, and
in most cases, I suspect at least some will agree.  If enough
developers find the new construct *less* readable, then it should
probably not be included.

> * how much more compact can the code be made by its use?

It's not only about making code more compact.  A good helper function
or macro also reduces the risk of bugs, since a mistyped
function/macro name will simply not compile.  In the specific case of
array sizes, a macro could crafted to ensure it can only be applied to
actual arrays, not pointers.  The latter is a bug I've seen on more
than one occasion.

> Its not really specific to macros ...
> And i do not know if this specific case is good or bad ...
> I simply do not find
> sizeof(array) / sizeof(array[0])
> particularely ugly ...

I'm not saying it's ugly.  It is, however, long enough that I (don't
know about others) have to pause and make sure it really does what it
appears to do on first glance.  Having a well-named macro do the same
thing would make this immediately obvious.

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