[Freeciv-Dev] (PR#9785) terrain type enum values cleanup

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=9785 >

Some of the enum tile_terrain_type values are used in wrong ways.

- The code shouldn't assume that T_UNKOWNN == T_COUNT.

- The code shouldn't assume that T_FIRST == T_ARCTIC.

- The code shouldn't assume that T_LAST == MAX_NUM_TERRAINS.

- MAX_NUM_TERRAINS should be T_COUNT, not T_LAST.

- The code shouldn't assume that T_LAST == T_COUNT (especially since it 
doesn't!).

Fixing these assumptions should make changing the enumeration a lot less 
bug-prone.

The only questionable thing is the return value of get_terrain_by_name. 
  The old version of the function returned T_COUNT if the terrain 
couldn't be found.  The new version returns T_UNKOWN.  However the 
callers are changed to do a != comparison against T_UNKNOWN rather than 
the old (bad) >= comparison.  A somewhat safer alternative might be to do a

   (t < T_FIRST || t >= T_COUNT)

comparison to see if we found a valid base terrain.

jason

Index: client/gui-sdl/mapview.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-sdl/mapview.c,v
retrieving revision 1.70
diff -u -r1.70 mapview.c
--- client/gui-sdl/mapview.c    10 Jul 2004 18:48:18 -0000      1.70
+++ client/gui-sdl/mapview.c    24 Aug 2004 02:52:11 -0000
@@ -99,7 +99,7 @@
 
 static SDL_Surface *pOcean_Tile;
 static SDL_Surface *pOcean_Foged_Tile;
-static SDL_Surface *pDithers[T_LAST][4];
+static SDL_Surface *pDithers[MAX_NUM_TERRAINS][4];
 
 static enum {
   NORMAL = 0,
@@ -3257,8 +3257,7 @@
   SDL_FillRect(pBuf, NULL, color);
   SDL_SetColorKey(pBuf, SDL_SRCCOLORKEY, color);
   
-  for (terrain = T_ARCTIC ; terrain < T_LAST ; terrain++)
-  {
+  for (terrain = T_FIRST; terrain < T_COUNT; terrain++) {
     
     if (terrain == T_RIVER || is_ocean(terrain) || terrain == T_UNKNOWN)
     {
@@ -3328,8 +3327,7 @@
   enum tile_terrain_type terrain;
   int i;
     
-  for (terrain = T_ARCTIC ; terrain < T_LAST ; terrain++)
-  {
+  for (terrain = T_FIRST; terrain < T_COUNT; terrain++) {
     for(i = 0; i < 4; i++)
     {
       FREESURFACE(pDithers[terrain][i]);
@@ -3345,8 +3343,8 @@
 {
   enum tile_terrain_type terrain;
   int i;
-  for (terrain = T_ARCTIC ; terrain < T_LAST ; terrain++)
-  {
+
+  for (terrain = T_FIRST; terrain < T_COUNT; terrain++) {
     for(i = 0; i < 4; i++)
     {
       pDithers[terrain][i] = NULL;
Index: common/terrain.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/terrain.c,v
retrieving revision 1.9
diff -u -r1.9 terrain.c
--- common/terrain.c    9 Jul 2004 18:52:36 -0000       1.9
+++ common/terrain.c    24 Aug 2004 02:52:12 -0000
@@ -24,7 +24,7 @@
 #include "support.h"
 #include "terrain.h"
 
-struct tile_type tile_types[T_LAST];
+struct tile_type tile_types[MAX_NUM_TERRAINS];
 
 /***************************************************************
 ...
@@ -34,18 +34,20 @@
   return &tile_types[type];
 }
 
-/***************************************************************
-...
-***************************************************************/
+/****************************************************************************
+  Return the terrain type matching the name, or T_UNKNOWN if none matches.
+****************************************************************************/
 enum tile_terrain_type get_terrain_by_name(const char * name)
 {
   enum tile_terrain_type tt;
+
   for (tt = T_FIRST; tt < T_COUNT; tt++) {
     if (0 == strcmp (tile_types[tt].terrain_name, name)) {
-      break;
+      return tt;
     }
   }
-  return tt;
+
+  return T_UNKNOWN;
 }
 
 /***************************************************************
Index: common/terrain.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/terrain.h,v
retrieving revision 1.18
diff -u -r1.18 terrain.h
--- common/terrain.h    21 Aug 2004 23:14:43 -0000      1.18
+++ common/terrain.h    24 Aug 2004 02:52:12 -0000
@@ -67,9 +67,14 @@
   T_LAST, /* last terrain type */
   T_ANY   /* A special flag that matches "any" terrain type. */
 };
+
+/* The first terrain value and number of base terrains.  This is used in
+ * loops.  T_COUNT may eventually be turned into a variable. */
 #define T_FIRST (T_ARCTIC)
-#define T_COUNT (T_UNKNOWN)
-#define MAX_NUM_TERRAINS (T_LAST)
+#define T_COUNT (T_TUNDRA + 1)
+
+/* A hard limit on the number of terrains; useful for static arrays. */
+#define MAX_NUM_TERRAINS (T_COUNT)
 
 /* Must match with terrain_flag_from_str in terrain.c. */
 enum terrain_flag_id {
@@ -93,7 +98,7 @@
 
 BV_DEFINE(bv_terrain_flags, TER_MAX);
 
-extern struct tile_type tile_types[T_LAST];
+extern struct tile_type tile_types[MAX_NUM_TERRAINS];
 
 /* General accessor functions. */
 struct tile_type *get_tile_type(enum tile_terrain_type type);
Index: server/ruleset.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/ruleset.c,v
retrieving revision 1.188
diff -u -r1.188 ruleset.c
--- server/ruleset.c    24 Aug 2004 01:59:43 -0000      1.188
+++ server/ruleset.c    24 Aug 2004 02:52:13 -0000
@@ -1183,7 +1183,7 @@
     k = 0;
     for (j = 0; j < count; j++) {
       b->terr_gate[k] = get_terrain_by_name(list[j]);
-      if (b->terr_gate[k] >= T_UNKNOWN) {
+      if (b->terr_gate[k] == T_UNKNOWN) {
        freelog(LOG_ERROR,
                "for %s terr_gate[%d] couldn't match terrain \"%s\" (%s)",
                b->name, j, list[j], filename);
@@ -1394,7 +1394,7 @@
          e->aff_terr = T_LAST;
        } else {
          e->aff_terr = get_terrain_by_name(item);
-         if (e->aff_terr >= T_UNKNOWN) {
+         if (e->aff_terr == T_UNKNOWN) {
            freelog(LOG_ERROR,
                    "for %s effect[%d].aff_terr couldn't match terrain \"%s\" 
(%s)",
                    b->name, j, item, filename);
Index: server/sanitycheck.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/sanitycheck.c,v
retrieving revision 1.46
diff -u -r1.46 sanitycheck.c
--- server/sanitycheck.c        13 Aug 2004 15:59:13 -0000      1.46
+++ server/sanitycheck.c        24 Aug 2004 02:52:13 -0000
@@ -53,7 +53,7 @@
     if (contains_special(special, S_IRRIGATION))
       assert(get_tile_type(terrain)->irrigation_result == terrain);
 
-    assert(terrain >= T_ARCTIC && terrain < T_UNKNOWN);
+    assert(terrain >= T_FIRST && terrain < T_COUNT);
   } whole_map_iterate_end;
 }