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

[pl]

On Tue, Feb 26, 2002 at 02:13:08AM +0100, Arpi wrote:

> > > > 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.
> 
> I still can't see the problem with this

To me, s isn't checked against NULL 8)
Anyway it will crash soon should your malloc start failing.

-- 
Best regards,
  pl