[MPlayer-dev-eng] [PATCH] libbs2b audio filter

[Uoti Urpala]

On Thu, 2009-03-05 at 19:20 +0300, Andrew Savchenko wrote:
> Hi,
> 
> On Thursday 05 March 2009 16:39, Uoti Urpala wrote:
> [...]
> > > Necessity of using void* pointers and pointer to function
> > > ensues from aiming to avoid six different play functions for
> > > different types of audio data or even worse alternative:
> > > switch statement inside single play function. Original
> > > processing functions from
> >
> > You could use char * for the pointer arithmetic.
> 
> Afaik char is not garantied to be exactly 8 bit width. I used 
> u_int8_t as Reimar suggested.

Your first sentence is true, but the second doesn't follow. What kind of
setup do you imagine where char would not be exactly 8 bits, but uint8_t
would work better for this use?

> > > +typedef struct af_bs2b_s
> >
> > I'd use the struct type directly leave leave out the typedef.
> 
> This is a rather common practice to use typedef to avoid explicit 
> usage of full structure type at every place. This doesn't hurt 
> performance and, imho, make it easier to read the code.

Of course it doesn't affect performance in any way. IMO it's easier to
read the code if type names are distinguishable and not arbitrary custom
typedefs.

> > > +    void (*bs2b)();
> >
> > You can't pretend that the function has no argument type by
> > leaving it out of the declaration. As long as the library
> > functions actually have different types it is NOT possible to
> > call them from a single location.
> 
> Why? It isn't fast definitely, but it should work anyway if number 
> and type of arguments will be compatible with those declared in 
> the function prototype of actually called function.

You are _not_ calling it with compatible argument types. If you were
then you could declare the function pointer with those correct types and
there would be no warnings. You are calling it with a "void *" second
argument, while the actual functions have incompatible argument types
like "float *" or "short *". You can give a void * as a parameter to a
function that has been declared to take any pointer type because the
compiler allows automatic conversion from the void * to the argument
type, but here such automatic conversion cannot happen because the
function pointer hasn't been declared with the correct type and so the
compiler doesn't know the actual type of the function called.

> > +        if (!sscanf(arg,"%i:%i", &opt[0], &opt[1]))
> > +        {
> >
> > Put opening brace on the same line as the "if".
> 
> Why? IMO this kind of indentation makes it easier to read the code.

Others disagree, and you're not going to be the only one reading the
source. K&R style is the closest to existing sources and most acceptable
standard style so that's what should be used.