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

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:
> 
> On Mon, Oct 08, 2001 at 01:50:41PM -0400, Jason Dorje Short wrote:

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

Yeah.  I'm looking at another macro that would allow writing

  DEC2HEX(map_get_tile(x, y)->special, 3)

instead of

  dec2hex[(map_get_tile(x, y)->special >> 12) & 0xf]

or (what is now used)

  dec2hex[(map_get_tile(x, y)->special & 0xf000) >> 12]

But, DEC2HEX is a bit of a misnomer.

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

One example:

http://www.freeciv.org/lxr/source/server/savegame.c#L158

If a line is not there, it is skipped.  This is done for each line. 
Other usages should (but do not) assert that each line exists (rather
than being a null pointer) and possibly that it is long enough as well.

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

No, the character is skipped entirely upon loading.  It is just included
as a convenience so that (1) indexing can be done with the x value
directly and (2) the file is more human-readable.  The only restriction
is that it can't be '\0', which would terminate the string.

My comment was supposed to indicate this, but apparently isn't
descriptive enough.  It would be easier to see if the loading macro were
also written.

If it makes you more comfortable I'll choose a different character,
although this will make the files less human-readable.

jason