[Freeciv-Dev] Re: sound patch v5

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Jan 27, 2002 at 03:43:04PM +0100, Per I. Mathisen wrote:
> On Sun, 27 Jan 2002, Raimar Falke wrote:
> > > ./configure: SDL: command not found
> >
> > This shouldn't happen.
> 
> What happened? What shouldn't happen?

That configure produce such an error message.

> Did configure abort?

No.

> What is the context of "SDL: command not found"?

As shown:

> > > checking for SDL - version >= 1.2.0...
> > > *** 'sdl-config --version' returned 1.1.8, but the minimum version
> > > *** of SDL required is 1.2.0. If sdl-config is correct, then it is
> > > *** best to upgrade to the required version.
> > > *** If sdl-config was wrong, set the environment variable SDL_CONFIG
> > > *** to point to the correct copy of sdl-config, and remove the file
> > > *** config.cache before re-running configure
> > > no
> > > ./configure: SDL: command not found
> > > checking for SDL/SDL_mixer.h... yes
> > > checking for Mix_OpenAudio in -lSDL_mixer... yes
> > > checking for SDL/SDL_mixer.h... yes
> > > checking for Mix_OpenAudio in -lSDL_mixer... yes

> > "./configure --enable-sound=esd" should work as expected.
> 
> Right. Disable the other sound systems when one chosen explicitly.
> 
> > gcc -DHAVE_CONFIG_H -I. -I. -I.. -I./include -I../common -I../intl       -g 
> > -O2 -Wall -c audio_sdl.c
> > audio_sdl.c:27:17: SDL.h: Datei oder Verzeichnis nicht gefunden
> > audio_sdl.c:28:23: SDL_mixer.h: Datei oder Verzeichnis nicht gefunden
> 
> That is strange. Above I got the impression that SDL was _not_ configured
> in. Then audio_sdl.c should not be compiled, either. To get it to compile,
> comment out "#define SDL" in config.h and recompile.

> > > +/* keep it open throughout */
> > > +struct section_file tagstruct, *tagfile = &tagstruct;
> > > +
> > > +/* dynamic plug-in control */
> > > +/* esd=1, sdl=2 */
> > > +int audio_plugin = -1;
> >
> > Shouldn't these be statics?
> ...
> > > +int sample_id[MAX_SAMPLES];
> > > +int music_id=-1;
> > > +int last_sample=0;
> > > +int sock = -1;
> >
> > Static?
> 
> Is there any point in declaring globals as static?

You can't reference them from outside. Another file could do a "extern
int sock;" and may access "sock" this way.

> Aren't they always?

No.

> > > +  assert(0);
> > > +  return 0; /* we should never get here */
> >
> > NDEBUG will bring us there. You may want to use "exit(1)".
> 
> The comment is "should never", not "can never". Why exit(1) when the
> program continues to be in a sane state despite the bug?

I have to look at the context in the next review.

> > > @@ -131,7 +133,10 @@
> > >      fprintf(stderr, _("  -p, --port PORT\tConnect to server port 
> > > PORT\n"));
> > >      fprintf(stderr, _("  -s, --server HOST\tConnect to the server at 
> > > HOST\n"));
> > > +    fprintf(stderr, _("  -a, --audio FILE\tRead audio tags from 
> > > FILE\n"));
> >
> > -a is already used. Also -s[ound] is used already. Also -n[oise].
> 
> What do you suggest, then? I can't say I like the current getopt regime.

You may choose "-A" or "-o" for sOund. Or anything except -[adhlmnpstv]

> > > @@ -719,5 +727,5 @@
> > >  {
> > >    int where = MW_OUTPUT; /* where to display the message */
> > > -
> > > +
> > >    if (packet->event >= E_LAST)  {
> > >      /* Server may have added a new event; leave as MW_OUTPUT */
> > > @@ -770,5 +779,5 @@
> > >  void handle_move_unit(struct packet_move_unit *packet)
> > >  {
> > > -  /* should there be something here? - mjd */
> > > +  assert(!packet); /* this packet should never get sent to a client */
> > >  }
> > >
> >
> > Not sound related.
> 
> Right. But the latter change should still go in - it took me some time to
> figure that one out, and it is a documentation bug.

You are right.

> > > +  assert(sound&&*sound);
> >
> > You use this idiom in some places. What about
> > "assert(sound && strlen(sound) > 0);"?
> 
> Isn't that saying the same in lot more words (and an additional function
> call)?

I expected this response. It wasn't a hard suggestion. Leave it.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "The two rules for success in life are:
  1) Never tell them everything you know."