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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Oct 08, 2001 at 01:50:41PM -0400, Jason Dorje Short wrote:
> 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.

This encoding of the special and the known field is ugly, very ugly.

I don't see any code which skips over data (well savegame.c is
huge). So I don't see the problem.

> 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";
>  
> +      if (is_normal_map_pos(x, y)) {                                         
>  \
> +     pbuf[x_itr] = get_xy_char;                                            \
> +      } else {                                                               
>  \
> +     pbuf[x_itr] = ' '; /* skipped over in loading */                      \
> +      }                                                                      
>  \

Wrong. ' ' is a valid terrain char. You have to think of another one.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  (On the statement print "42 monkeys"+"1 snake"): BTW, both perl and Python
  get this wrong. Perl gives 43 and Python gives "42 monkeys1 snake", when 
  the answer is clearly "41 monkeys and 1 fat snake".  
    -- Jim Fulton, 10 Aug 1999