[Freeciv-Dev] Re: Reproducable core dump (PR#1051)

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

Petr Baudis wrote:

> > Index: server/gotohand.c
> > ===================================================================
> > RCS file: /home/freeciv/CVS/freeciv/server/gotohand.c,v
> > retrieving revision 1.122
> > diff -u -r1.122 gotohand.c
> > --- server/gotohand.c   2001/10/08 12:11:17     1.122
> > +++ server/gotohand.c   2001/11/30 20:12:27
> > @@ -38,6 +38,15 @@
> > /* These are used for airplane GOTOs with waypoints */
> > #define MAXFUEL 100
> >
> > +/* These should be either true or false.  They are used for finding routes
> > +    for airplanes - the airplane doesn't want to fly through occupied
> > +    territory, but most territory will be either fogged or unknown entirely.
> > +    See airspace_looks_usable().  Note that this is currently only used
> > +    by the move-counting function air_can_move_between(), not by
> > +    the path-finding function (whatever that may be). */
> This comment's format looks odd. But never mind ;-).

Raimar has been reformatting such comments himself - but presumably 
_after_ he runs indent over the patch.  I'd be quite willing to do this 
part myself, but it should only be done right before the patch is 
committed - up until that point, having indent work on the comment is 
good; otherwise you have to reformat by hand (sometimes many times) and 
that's way too tedious.
I generally prefer this style of comment, because indent will reformat 
it as necessary.  For a project like freeciv, though, we probably want 
to avoid such reformatting so that cvs annotate will work properly.  But 
still, it should only be done right before the patch is applied IMO.

> > +  /* If the tile's unknown, we (may) assume it's empty. */
> > +  if (ai_handicap(pplayer, H_MAP) &&
> This is odd. If pplayer is not AI, ai_handicap() will return -1, which is
> nonzero so it will always return AIR_ASSUMES_UNKNOWN_SAFE for human players.
> If it's an intent, it should be stated some comment clearly, otherwise you
> should first test if the player is AI-controlled.

Well, I thought this out clearly, but I suppose a comment is in order. 
As I understand ai_handicap, it returns TRUE if the player is 
handicapped in the given way - in this case, handicapped by not being 
able to see the whole map.  For human players, it will always return 
true - which is the intent.  An ai-player check would just duplicate the 
work already done in ai_handicap - which I believe is the point Tony was 
trying to make earlier.
The point of AIR_ASSUMES_UNKNOWN_SAFE is to just guess one way or the 
other whether the tile is safe when it's unknown.  If the handicap check 
returns FALSE, this won't be done and execution will continue on down.
The H_FOG case is the same.



Thanks for the quick feedback.  I'll send along another patch (with the

new comment) as soon as I hear from Gregory and/or Raimar.

jason