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

[D Richard Felker III]

the issue is that you are not following the established procedures for
mplayer development. patches are supposed to be kept to a minimum and
make only functional changes, not cosmetic ones. large cosmetic
changes make it hard to go back and reverse patches that turn out to
be bad later on, and its hard for developers to stay familiar with
their own code when someone else is reorganizing it and renaming
variables all the time.

some of the ideas you have put forth make sense when you are working
alone or with a tight-knit group of just a few developers all on-site
fulltime at the same location, e.g. in a commercial setting. however,
they do not work well with the kind of development style/cvs usage
that has brought us mplayer. i am very amazed that a project like
mplayer is able to work with so many developers all working on the
live cvs tree at once -- it's fairly rare for significant bugs to be
introduced, and very rare for the cvs version to fail to compile at
all, unlike with many other projects. imo, part of what's made this
work is arpi being anal about what types of patches/commits are and
are not allowed.

finally, regardless of the relative merits of different development
styles, you are severely lacking in social skills. you don't go butt
into a project and tell all the current developers that their way of
doing things is stupid -- it won't get you anywhere.

i hope that clarifies some of your recurring questions.

rich


On Mon, Feb 25, 2002 at 05:43:49PM -0700, Mike Melanson wrote:
> 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
> 
> _______________________________________________
> MPlayer-dev-eng mailing list
> MPlayer-dev-eng at mplayerhq.hu
> http://mplayerhq.hu/mailman/listinfo/mplayer-dev-eng