[vorbis-dev] ogg123 remote interface

[Segher Boessenkool]

Joost Remijn wrote:
> 
> On Sun, 6 Oct 2002, Segher Boessenkool wrote:
> 
> > Just some comments on getting-rid-of-warnings:
> >
> > > remove-globals.patch
> > >   remove the many global variables from ogg123.c
> >
> > bad idea for ogg123, imho
> 
> Why would it be a bad idea to remove some globals. One of them even had a
> comment wich said it should be removed after RC3.

Because, imho, globals are *good* for some things, like, erm, "application
global variables": there's no go reason to not make application globals
global.  Applications are not libraries.

> > > make-character-pointers-const.patch
> >
> > useless.  makes no difference at all for generated code.  const in C does
> > not mean the same as const in C++.  it just clobbers up the code.
> 
> Here is the explanation from the gcc info page :
> 
> `-Wwrite-strings'
>      Give string constants the type `const char[LENGTH]' so that
>      copying the address of one into a non-`const' `char *' pointer
>      will get a warning.  These warnings will help you find at compile
>      time code that can try to write into a string constant, but only
>      if you have been very careful about using `const' in declarations
>      and prototypes.  Otherwise, it will just be a nuisance; this is
>      why we did not make `-Wall' request these warnings.
> 
> So there is probably no difference in the generated binary, but the code
> is cleaner and easier to debug if someone tries to change these strings.

The code is cluttered with useless "const" strings, so certainly not
cleaner!  And, as GCC's doc says, it's a big nuissance.

> > > remove-some-compiler-warnings.patch
> > >   Fixes for some compiler warnings. Unused variable buf commented out.
> >
> > remove or don't remove, but don't comment out ;)
> 
> I commented it out because it looked as if that function could be filled
> in later. Should be removed then.

There's always CVS if you need old code :)

> > >   char * initialized to 0 instead of "".
> >
> > that means something different -- i hope you checked it doesn't
> > actually matter
> 
> yep i checked. Does it ever make sense to assign "" to a pointer?

Often, as another poster already remarked (with an example).

> > > remove-shadowed-variables.patch
> > >   Fix some places in the code where variables are "shadow"ed. These
> > >   show up as compiler warnings when compiling with -Wshadow.
> >
> > bad idea, imnsho.
> 
> One example of this was a local variable called optind. It can be
> confusing sometimes if people see optind being used and assume it's the
> global one. That's why i think it was a good idea.

It's a good idea if you're able to choose better names for the variables,
sure.  'optind' isn't a particularly well-chosen name.

> > > make-playlist-array-null-terminated.patch
> > >   Make the playlist NULL terminated. This prevents us from carrying around
> > >   the number of items in the playlist later on.
> >
> > depends -- what about shuffling?  a single int isn't expensive, reading a list
> > _is_.
> 
> It made the code cleaner. It adds one complete list-walk when shuffling
> indeed. The reason i did it was that it became possible to pass the
> playlist to a function without also passing the int. Same for returning
> the playlist from playlist_create().

If this only happens when shuffling, it's fine with me, I guess ;)

<p>Either way, I'm not the ogg123 maintainer, so let's see what he decides.

<p>Segher

<p>--- >8 ----
List archives:  http://www.xiph.org/archives/
Ogg project homepage: http://www.xiph.org/ogg/
To unsubscribe from this list, send a message to 'vorbis-dev-request at xiph.org'
containing only the word 'unsubscribe' in the body.  No subject is needed.
Unsubscribe messages sent to the list will be ignored/filtered.