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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

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:
> 
> #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.

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

> 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.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "If at first you don't succeed... well so much for skydiving."