[MPlayer-dev-eng] [PATCH] libbs2b: new api

[Uoti Urpala]

On Tue, 2009-04-21 at 00:33 +0400, Andrew Savchenko wrote:
> On Monday 20 April 2009, Uoti Urpala wrote:
> > The documentation specifies certain parameter ranges, but the
> > actual code uses range defines from the library. I don't know
> > whether those are supposed to always stay constant; perhaps
> > there should be some mention of that?
> 
> I don't know either. Anyway it is supposed to keep up-to-date sync 
> with library, so if changes will occur they should be fixed 
> relatively shortly. That's why I'm not sure this issue is worth to 
> be mentioned in the manual page, of course this point of view is 
> discussable.

Well, I don't see any obvious better way, and I guess it's not worth
spending more energy on.

> > Some code formatting issues that occur multiple times:
> >
> > +           "[bs2b] Feed level must be in range [%i..%i], but
> > current value is %i.\n",
> >
> > You should use "%d" instead of "%i" like most of MPlayer code
> > (and most C code I've seen in general).
> 
> Hm, I really can't see how it differs, standard allows both of 
> them. Anyway there is zero difference for the bytecode, so let it 
> be as you wish ).

Yes it is also allowed by the standard (I listed it under code
formatting issues rather than functionality ones), but there's no reason
to use a random mix of both and "%d" is the more common one.

> > +            else
> > +            if (!strcmp(s->profile,"cmoy"))
> >
> > "else if" should be on one line.
> 
> No, no, no. The idea is to keep all if statements at the same 
> alignment level, otherwise it will be mess to read. Currently they 
> have nice and easy-to-understand-the-code indentation. Statements 
> are logically equal and there is no reason to accent one of them. 
> You may consider this as swith statement for strings.
> 
> Yes, I know this is not strict K&R indentation but sometimes logics 
> should override common rules.

"switch statement with if" is a common case, not in any way an
exception. Your version is not objectively better. You can make up
arguments at least as good as your "logically equal" against it if you
want. I find it uglier in practice too.

Inventing your own code formatting rules for common cases is simply a
bad idea. This is a common case, and K&R style does clearly specify the
formatting. You will not be able to come up with any objective reasons
why writing it some other way would be so much more
intuitive/logical/whatever that it would override the benefit of keeping
a consistent style.


> > +                bs2b_set_level(s->filter,BS2B_JMEIER_CLEVEL);
> >
> > Space missing after ",".
> 
> Ok.

Still has at least this:
+            bs2b_set_level_fcut(s->filter,s->fcut);
+        if (s->feed)
+            bs2b_set_level_feed(s->filter,s->feed);