[Freeciv-Dev] Re: Cleaning up server/savegame.c

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:
> 
> On Mon, Oct 08, 2001 at 01:13:07AM -0400, Jason Dorje Short wrote:
> > Raimar suggested something like the following macro to help clean up
> > savegame.c:

> From
>  $ grep -n "secfile_insert_str.*y" savegame.c
> 
> Make two macros: eon for this group
> 
> 248:    secfile_insert_str(file, pbuf, "map.n%03d", y);
> 520:    secfile_insert_str(file, pbuf, "map.t%03d", y);
> 537:    secfile_insert_str(file, pbuf, "map.l%03d", y);
> 546:    secfile_insert_str(file, pbuf, "map.u%03d", y);
> 555:    secfile_insert_str(file, pbuf, "map.n%03d", y);
> 566:      secfile_insert_str(file, pbuf, "map.f%03d", y);
> 574:      secfile_insert_str(file, pbuf, "map.a%03d", y);
> 582:      secfile_insert_str(file, pbuf, "map.b%03d", y);
> 590:      secfile_insert_str(file, pbuf, "map.c%03d", y);
> 598:      secfile_insert_str(file, pbuf, "map.d%03d", y);
> 606:      secfile_insert_str(file, pbuf, "map.e%03d", y);
> 614:      secfile_insert_str(file, pbuf, "map.g%03d", y); /* f already taken! 
> */
> 622:      secfile_insert_str(file, pbuf, "map.h%03d", y);
> 630:      secfile_insert_str(file, pbuf, "map.i%03d", y);
> 
> and one for this group
> 
> 1732:      secfile_insert_str(file, pbuf, "player%d.map_t%03d", plrno, y);
> 1741:      secfile_insert_str(file, pbuf, "player%d.map_l%03d",plrno,  y);
> 1750:      secfile_insert_str(file, pbuf, "player%d.map_u%03d", plrno,  y);
> 1759:      secfile_insert_str(file, pbuf, "player%d.map_n%03d", plrno, y);
> 1768:      secfile_insert_str(file, pbuf, "player%d.map_ua%03d",plrno,  y);
> 1777:      secfile_insert_str(file, pbuf, "player%d.map_ub%03d", plrno,  y);
> 1786:      secfile_insert_str(file, pbuf, "player%d.map_uc%03d", plrno, y);
> 1795:      secfile_insert_str(file, pbuf, "player%d.map_ud%03d", plrno, y);

Very sensible.  See the attached demo patch.

> > For the record, the load macro won't be quite as pretty, something like
> >
> > #define load_map_data(secfile_lookup_line, set_xy_char, x_itr, y_itr,
> > ch)   \
> > {                                                                    \
> >   int x_itr, y_itr;                                                  \
> >   for (y_itr=0; y_itr < map.ysize; y_itr++) {                        \
> >     char *secline= secfile_lookup_line;                              \
> >     for (x=0; x<map.xsize; x++) {                                    \
> >       char ch = secline[x];                                         \
> >       set_xy_char;                                                   \
> >     }                                                                \
> >   }                                                                  \
> > }
> >
> > and leading to constructs like
> >
> >
> >   load_map_data(secfile_lookup_str(file, "map.l%03d", y),
> >               if(isxdigit(ch)) {
> >                 map_get_tile(x, y)->special=ch-(isdigit(ch) ? '0' : 
> > ('a'-10));
> >                       } else if(ch!=' ') {
> >                 freelog(LOG_FATAL, "unknown special flag(lower) (map.l) in 
> > map "
> >                                       "at position(%d,%d): %d '%c'", x, y, 
> > ch, ch);
> >                 exit(1);
> >                       }, x, y, ch);
> >
> > which is not all that much of an improvement over existing code.
> 
> I solution would be to have a "decode_hex" method which also does the
> error checking.

If we assume hex values then we can do much better, not only removing
the encoding/decoding code but also combining all 4 segments necessary
to store an integer into one macro.

Unfortunately this is not always the case, and the number of macros
would then grow.  Additionally, in some cases during load the code skips
over data if it's not present, while not in others - this would require
an addional macro or extra parameter to the existing one.  In summary,
it's not pretty.

jason

? rc
Index: common/map.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
retrieving revision 1.94
diff -u -r1.94 map.c
--- common/map.c        2001/10/08 12:11:16     1.94
+++ common/map.c        2001/10/08 17:50:13
@@ -1314,6 +1314,19 @@
 }
 
 /**************************************************************************
+Returns TRUE iff the map position is normal.  "Normal" here means that it
+is both a real/valid coordinate set and that the coordinates are in their
+canonical/proper form.  In plain English: the coordinates must be on the
+map.
+**************************************************************************/
+int is_normal_map_pos(int x, int y)
+{
+  int x1 = x, y1 = y;
+
+  return normalize_map_pos(&x1, &y1) && x1 == x && y1 == y;
+}
+
+/**************************************************************************
 Normalizes the map position. Returns TRUE if it is real, FALSE otherwise.
 **************************************************************************/
 int normalize_map_pos(int *x, int *y)
Index: common/map.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
retrieving revision 1.95
diff -u -r1.95 map.h
--- common/map.h        2001/10/08 12:11:16     1.95
+++ common/map.h        2001/10/08 17:50:13
@@ -231,6 +231,7 @@
 enum known_type tile_is_known(int x, int y);
 int check_coords(int *x, int *y);
 int is_real_tile(int x, int y);
+int is_normal_map_pos(int x, int y);
 int normalize_map_pos(int *x, int *y);
 void nearest_real_pos(int *x, int *y);
 
Index: server/savegame.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/savegame.c,v
retrieving revision 1.33
diff -u -r1.33 savegame.c
--- server/savegame.c   2001/10/03 09:16:56     1.33
+++ server/savegame.c   2001/10/08 17:50:15
@@ -54,6 +54,38 @@
 static const char dec2hex[] = "0123456789abcdef";
 static const char terrain_chars[] = "adfghjm prstu";
 
+/* This loops over the entire map to create save data, and stores it in the
+   file under the given secfile structure.  It works like this:
+     x_itr, y_itr -> x and y iterator variables
+     file -> the section_file we're saving to
+     get_xy_char -> a line of code that returns the character for each
+       (x, y) coordinate.  It should use the x/y iterator variables
+       appropriately.
+     secfile_index -> the indexor to the secfile, passed to secfile_insert.
+       It takes y_itr as an argument, so it should have a %03d in it. */
+#define save_map_data(secfile, get_xy_char, secfile_insert_line, x_itr, y_itr) 
      \
+{                                                                             \
+  char pbuf[map.xsize+1];                                                     \
+  int x_itr, y_itr;                                                           \
+  for (y_itr=0; y_itr<map.ysize; y_itr++) {                                   \
+    for (x_itr=0; x_itr<map.xsize; x_itr++) {                                 \
+      if (is_normal_map_pos(x, y)) {                                          \
+       pbuf[x_itr] = get_xy_char;                                            \
+      } else {                                                                \
+       pbuf[x_itr] = ' '; /* skipped over in loading */                      \
+      }                                                                       \
+    }                                                                         \
+    pbuf[map.xsize] = '\0';                                                   \
+    secfile_insert_line;                                                      \
+  }                                                                           \
+}
+
+#define save_reg_map_data(secfile, get_xy_char, secfile_name, x_itr, y_itr)   \
+       save_map_data(secfile, get_xy_char, secfile_insert_str(secfile, pbuf, 
secfile_name, y_itr), x_itr, y_itr)
+#define save_plr_map_data(secfile, get_xy_char, secfile_name, x_itr, y_itr, 
plrno) \
+       save_map_data(secfile, get_xy_char, secfile_insert_str(secfile, pbuf, 
secfile_name, plrno, y_itr), x_itr, y_itr)
+
+
 /* Following does not include "unirandom", used previously; add it if
  * appropriate.  (Code no longer looks at "unirandom", but should still
  * include it when appropriate for maximum savegame compatibility.)
@@ -236,18 +268,9 @@
 ***************************************************************/
 static void map_rivers_overlay_save(struct section_file *file)
 {
-  char *pbuf = fc_malloc(map.xsize+1);
-  int x, y;
-  
-  /* put "next" 4 bits of special flags */
-  for(y=0; y<map.ysize; y++) {
-    for(x=0; x<map.xsize; x++) {
-      pbuf[x]=dec2hex[(map_get_tile(x, y)->special&0xf00)>>8];
-    }
-    pbuf[x]='\0';
-    secfile_insert_str(file, pbuf, "map.n%03d", y);
-  }
-  free(pbuf);
+  save_reg_map_data(file,
+               dec2hex[(map_get_tile(x, y)->special&0xf00)>>8],
+               "map.n%03d", x, y);
 }
 
 /***************************************************************
@@ -494,8 +517,7 @@
 ***************************************************************/
 static void map_save(struct section_file *file)
 {
-  int i, x, y;
-  char *pbuf=fc_malloc(map.xsize+1);
+  int i;
 
   /* map.xsize and map.ysize (saved as map.width and map.height)
    * are now always saved in game_save()
@@ -512,126 +534,66 @@
   }
     
   /* put the terrain type */
-  for(y=0; y<map.ysize; y++) {
-    for(x=0; x<map.xsize; x++)
-      pbuf[x]=terrain_chars[map_get_tile(x, y)->terrain];
-    pbuf[x]='\0';
+  save_reg_map_data(file, terrain_chars[map_get_tile(x, y)->terrain],
+                       "map.t%03d", x, y);
 
-    secfile_insert_str(file, pbuf, "map.t%03d", y);
-  }
-
   if (!map.have_specials) {
     if (map.have_rivers_overlay) {
       map_rivers_overlay_save(file);
     }
-    free(pbuf);
     return;
   }
 
   /* put lower 4 bits of special flags */
-  for(y=0; y<map.ysize; y++) {
-    for(x=0; x<map.xsize; x++)
-      pbuf[x]=dec2hex[map_get_tile(x, y)->special&0xf];
-    pbuf[x]='\0';
+  save_reg_map_data(file, dec2hex[map_get_tile(x, y)->special&0xf],
+                       "map.l%03d", x, y);
 
-    secfile_insert_str(file, pbuf, "map.l%03d", y);
-  }
-
   /* put upper 4 bits of special flags */
-  for(y=0; y<map.ysize; y++) {
-    for(x=0; x<map.xsize; x++)
-      pbuf[x]=dec2hex[(map_get_tile(x, y)->special&0xf0)>>4];
-    pbuf[x]='\0';
-
-    secfile_insert_str(file, pbuf, "map.u%03d", y);
-  }
+  save_reg_map_data(file, dec2hex[(map_get_tile(x, y)->special&0xf0)>>4],
+                   "map.u%03d", x, y);
 
   /* put "next" 4 bits of special flags */
-  for(y=0; y<map.ysize; y++) {
-    for(x=0; x<map.xsize; x++)
-      pbuf[x]=dec2hex[(map_get_tile(x, y)->special&0xf00)>>8];
-    pbuf[x]='\0';
-
-    secfile_insert_str(file, pbuf, "map.n%03d", y);
-  }
+  save_reg_map_data(file, dec2hex[(map_get_tile(x, y)->special&0xf00)>>8],
+                   "map.n%03d", x, y);
 
   secfile_insert_int(file, game.save_options.save_known, "game.save_known");
   if (game.save_options.save_known) {
     /* put "final" 4 bits of special flags */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[(map_get_tile(x, y)->special&0xf000)>>12];
-      pbuf[x]='\0';
-
-      secfile_insert_str(file, pbuf, "map.f%03d", y);
-    }
+    save_reg_map_data(file, dec2hex[(map_get_tile(x, y)->special&0xf000)>>12],
+                       "map.f%03d", x, y);
 
     /* put bit 0-3 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[(map_get_tile(x, y)->known&0xf)];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.a%03d", y);
-    }
+    save_reg_map_data(file, dec2hex[(map_get_tile(x, y)->known&0xf)],
+                       "map.a%03d", x, y);
 
     /* put bit 4-7 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[((map_get_tile(x, y)->known&0xf0))>>4];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.b%03d", y);
-    }
+    save_reg_map_data(file, dec2hex[((map_get_tile(x, y)->known&0xf0))>>4],
+                       "map.b%03d", x, y);
 
     /* put bit 8-11 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[((map_get_tile(x, y)->known&0xf00))>>8];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.c%03d", y);
-    }
+    save_reg_map_data(file, dec2hex[((map_get_tile(x, y)->known&0xf00))>>8],
+                       "map.c%03d", x, y);
 
     /* put bit 12-15 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[((map_get_tile(x, y)->known&0xf000))>>12];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.d%03d", y);
-    }
+    save_reg_map_data(file, dec2hex[((map_get_tile(x, y)->known&0xf000))>>12],
+                     "map.d%03d", x, y);
 
     /* put bit 16-19 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[((map_get_tile(x, y)->known&0xf0000))>>16];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.e%03d", y);
-    }
+    save_reg_map_data(file, dec2hex[((map_get_tile(x, y)->known&0xf0000))>>16],
+                       "map.e%03d", x, y);
 
     /* put bit 20-23 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[((map_get_tile(x, y)->known&0xf00000))>>20];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.g%03d", y); /* f already taken! */
-    }
+    save_reg_map_data(file, dec2hex[((map_get_tile(x, 
y)->known&0xf00000))>>20],
+                       "map.g%03d", x, y);
 
     /* put bit 24-27 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[((map_get_tile(x, y)->known&0xf000000))>>24];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.h%03d", y);
-    }
+    save_reg_map_data(file, dec2hex[((map_get_tile(x, 
y)->known&0xf000000))>>24],
+                     "map.h%03d", x, y);
 
     /* put bit 28-31 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[((map_get_tile(x, y)->known&0xf0000000))>>28];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.i%03d", y);
-    }
+    save_reg_map_data(file, dec2hex[((map_get_tile(x, 
y)->known&0xf0000000))>>28],
+                     "map.i%03d", x, y);
   }
-  
-  free(pbuf);
 }
 
 /***************************************************************
@@ -1458,10 +1420,9 @@
 static void player_save(struct player *plr, int plrno,
                        struct section_file *file)
 {
-  int i, x, y;
+  int i;
   char invs[A_LAST+1];
   struct player_spaceship *ship = &plr->spaceship;
-  char *pbuf=fc_malloc(map.xsize+1);
 
   secfile_insert_str(file, plr->name, "player%d.name", plrno);
   secfile_insert_str(file, plr->username, "player%d.username", plrno);
@@ -1724,78 +1685,44 @@
   if (game.fogofwar
       && game.save_options.save_private_map) {
     /* put the terrain type */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=terrain_chars[map_get_player_tile(x, y, plr)->terrain];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_t%03d", plrno, y);
-    }
-    
+    save_plr_map_data(file,
+                 terrain_chars[map_get_player_tile(x, y, plr)->terrain],
+                  "player%d.map_t%03d", x, y, plrno);
+
     /* put lower 4 bits of special flags */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-      pbuf[x]=dec2hex[map_get_player_tile(x, y, plr)->special&0xf];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_l%03d",plrno,  y);
-    }
-    
+    save_plr_map_data(file,
+                  dec2hex[map_get_player_tile(x, y, plr)->special&0xf],
+                 "player%d.map_l%03d", x, y, plrno);
+
     /* put upper 4 bits of special flags */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[(map_get_player_tile(x, y, plr)->special&0xf0)>>4];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_u%03d", plrno,  y);
-    }
-    
+    save_plr_map_data(file,
+                  dec2hex[(map_get_player_tile(x, y, plr)->special&0xf0)>>4],
+                  "player%d.map_u%03d", x, y, plrno);
+
     /* put "next" 4 bits of special flags */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[(map_get_player_tile(x, y, plr)->special&0xf00)>>8];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_n%03d", plrno, y);
-    }
-    
+    save_plr_map_data(file,
+                  dec2hex[(map_get_player_tile(x, y, plr)->special&0xf00)>>8],
+                 "player%d.map_n%03d", x, y, plrno);
+
     /* put lower 4 bits of updated */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[map_get_player_tile(x, y, plr)->last_updated&0xf];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_ua%03d",plrno,  y);
-    }
-    
+    save_plr_map_data(file,
+                 dec2hex[map_get_player_tile(x, y, plr)->last_updated&0xf],
+                 "player%d.map_ua%03d", x, y, plrno);
+
     /* put upper 4 bits of updated */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[(map_get_player_tile(x, y, plr)->last_updated&0xf0)>>4];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_ub%03d", plrno,  y);
-    }
-    
+    save_plr_map_data(file,
+                 dec2hex[(map_get_player_tile(x, y, 
plr)->last_updated&0xf0)>>4],
+                 "player%d.map_ub%03d", x, y, plrno);  
+
     /* put "next" 4 bits of updated */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[(map_get_player_tile(x, y, 
plr)->last_updated&0xf00)>>8];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_uc%03d", plrno, y);
-    }
-    
+    save_plr_map_data(file,
+                 dec2hex[(map_get_player_tile(x, y, 
plr)->last_updated&0xf00)>>8],
+                 "player%d.map_uc%03d", x, y, plrno);
+
     /* put "yet next" 4 bits of updated */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[(map_get_player_tile(x, y, 
plr)->last_updated&0xf000)>>12];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_ud%03d", plrno, y);
-    }
-    
-    free(pbuf);
+    save_plr_map_data(file,
+                 dec2hex[(map_get_player_tile(x, y, 
plr)->last_updated&0xf000)>>12],
+                 "player%d.map_ud%03d", x, y, plrno);
     
     if (1) {
       struct dumb_city *pdcity;