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

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Raimar suggested something like the following macro to help clean up
savegame.c:

#define save_map_data(file, get_xy_char, secfile_name, x_itr, y_itr) \
{                                                                    \
  char pbuf[map.xsize+1];                                            \
  int x, y;                                                          \
  for (y=0; y<map.ysize; y++) {                                      \
    for (x=0; x<map.xsize; x++) {                                    \
      pbuf[x] = get_xy_char;                                         \
    }                                                                \
    pbuf[map.xsize] = '\0';                                          \
    secfile_insert_str(file, pbuf, secfile_name, y);                 \
  }                                                                  \
}

With a similar macro for loading map data.  This allows the following
code:

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);
}

To be changed into

static void map_rivers_overlay_save(struct section_file *file)
{
  save_map_data(file,
                dec2hex[(map_get_tile(x, y)->special&0xf00)>>8],
                "map.n%03d", x, y);
}

There are 44 iterations over the whole map in savegame.c, most or all of
which can be replaced by either the save or load macro.


However, there's a problem.  The secfile_insert_str line isn't constant;
for instance we have

    /* 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);
    }

which can obviously not be treated by the above macro.  The simplest
answer (solution#1) is just to pass this entire line to the macro, but
then we all of the formations are complicated like

  save_map_data(dec2hex[(map_get_tile(x, y)->special&0xf00)>>8],
                secfile_insert_str(file, pbuf, "map.n%03d", y), x, y, pbuf);

Which isn't as good.

Now, this could be turned into a more general iterator macro
(solution#2) like:

#define line_map_iterate(x_itr, y_itr, tile_cmd, line_cmd) \
{                                                          \
  int x, y;                                                \
  for (y=0; y<map.ysize; y++) {                            \
    for (x=0; x<map.xsize; x++) {                          \
      tile_cmd;                                            \
    }                                                      \
    line_cmd;                                              \
  }                                                        \
}

But this would still require a wrapper macro for best effect (trust me
on this one) and might not even be used elsewhere.

Or, we could just treat these extra cases separately (solution#3).


My inclination is to go with solution#1, but it's not as pretty as I'd
hoped.  We could assume x and y to be the iterator names, which would
make things slightly simpler.


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.

jason