Complete.Org: Mailing Lists: Archives: freeciv-dev: November 2001:
[Freeciv-Dev] Re: Reproducable core dump (PR#1051)

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

[Top] [All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
To: freeciv-dev@xxxxxxxxxxx
Cc: bugs@xxxxxxxxxxxxxxxxxxx
Subject: [Freeciv-Dev] Re: Reproducable core dump (PR#1051)
From: jdorje@xxxxxxxxxxxxxxxxxxxxx
Date: Fri, 30 Nov 2001 14:43:30 -0800 (PST)

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.


[Prev in Thread] Current Thread [Next in Thread]