[MPlayer-dev-eng] Re: [PATCH] Development (Was: Clean up demuxers)

[Mike Melanson]

On Tue, 26 Feb 2002, Arpi wrote:

> while you don't understand how does the whole demuxer thingie work, you
> shouldn't do any cleanup patch for that, as it's mostly broken...
> 
> it's not well documented, but that few people working on that knows it well
> so it isn't requires now.

	Let me see if I read that right: The demuxing subsystem is
convoluted and difficult to understand (even broken), but the people who
work with it already understand it, so there's no reason to rework it and
simplify it?

> > We're not talking about STYLE here. Code like:
> > 
> >   if(! IS_RUNNING(config)) {
> >     if(strcasecmp(opt,"vcd") == 0) {
> >       char* s;
> >       if(!param)
> >         return ERR_MISSING_PARAM;
> >       s = (char*)malloc((strlen(param) + 6 + 1)*sizeof(char));
> >       sprintf(s,"vcd://%s",param);
> >       entry = play_tree_new();
> >       play_tree_add_file(entry,s);
> >       free(s);
> >     } else if(strcasecmp(opt,"dvd") == 0) {
> > 
> > is a punishable offense in a few countries. If you cannot accept cleanup
> 
> what's the problem with this? it is not my code, anyway it looks ok for me.

	The issue that I notice right away is the declaration of new
variables. Though it's not that big of a deal in the above example (it's
natural to declare a new scope at the start of an 'if'), there is a big
problem with reckless declaration of new scopes at random places in the
code simply because the programmer wanted another local variable right
there. Now that's just sloppy.

> > > and renaming variables is really not welcomed. it makes impossible 
to
> > > replace demuxer.c with older version, or even merge with developers
> > > branches. it's simply codmetics change and won't be allowed here.
> > 
> > So you have troubles using development tools? Say it so and I might be
> > able to live with that. Otherwise this is simply FUD.
> 
> why?

	I don't understand the original issue here: What's the big deal
about renaming variables? Ideally, if a function is a good citizen and
uses all its own local variables, it shouldn't matter if it gets replaced
with a different version that has completely different variable names.
An older version of the code from CVS should still drop into the same
spot and compile fine. That is, unless the function relied on a mess of
global variables...but that would be poor coding practice...:)

> you seems to be a tipical 'doesn't matter if it won't work or slower,
> or breaks compatibility, but the source looks nicer' programmer (not coder).

	A'rpi, don't write this guy off so quickly. I'm sure he wants it
to work, and good coding practices rarely hurt application performance. If
they affect speed at all, they affect the speed of development because new
coders can understand the architecture quicker and existing developers can
find bugs faster. And about breaking compatibility? With what?
Ourselves?...:)

	I wasn't completely following the thread, but wasn't his original
intention to help us clean up the demuxing subsystem? We both know that
subsystem needs help (almost as much as the decoding subsystem needs help,
but that's another thread...:)

--
	-Mike Melanson