[Freeciv-Dev] PATCH: LOAD_MAP_DATA

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

This is a preleminary patch introducing the LOAD_MAP_DATA macro (see the
previous discussion about SAVE_MAP_POS).

Good things about it: it is topology-independent and MUCH shorter than
current code.

Problems: The basic form of the macro is

#define LOAD_MAP_DATA(secfile_lookup_line, set_xy_char) ...

which leads to ugly code constructs like

    LOAD_MAP_DATA(secfile_lookup_str(file, "player%d.map_t%03d", plrno,
y),
                  {
                    char *pch = strchr(terrain_chars, ch);
                    if(!pch) {
                      freelog(LOG_FATAL, "unknown terrain type (map.t) in map "
                        "at position (%d,%d): %d '%c'", x, y, ch, ch);
                      exit(1);
                    }
                    map_get_player_tile(x, y, plr)->terrain=pch-terrain_chars;
                  }
    );

Fortunately, code like this is only needed in 2 locations [1]; in the
other 20 or so spots there is much simpler code like


    LOAD_MAP_DATA(secfile_lookup_str(file, "player%d.map_ud%03d", plrno,
y),
                  map_get_player_tile(x, y, plr)->last_updated |= HEX2DEC(ch, 
3));

Here HEX2DEC (which is a pretty ugly macro itself) has simplified all of
the work.

As with the SAVE_MAP_DATA, there are basically two forms: one which
takes a plrno and one which does not.  Two separate macros could be
written for these two forms, but I'm pretty indifferent to it since it
wouldn't really simplify things that much.  There's another problem with
this approach: some code uses secfile_lookup_str and some uses
secfile_lookup_str_default.  Since the macro's usage is only two lines
as it is, I'm pretty content with it.

Note that it's much harder to verify the correctness of the loading
code, since you can't just make sure the new savegames are identical. 
This seems to load my existing games just fine, but we'll need to do a
pretty strong review before committing it.

Oh, one other thing: some of the current (CVS) code skips over map lines
that don't exist.  The LOAD_MAP_POS macro doesn't do this, instead it
asserts that the line exists and then continues blindly.  I don't know
if it's actually necessary to skip empty lines (presumably it's a
backwards-compatibility thing), but I think another parameter, force,
needs to be added to the macro that will control this.

Let me know what you think.


[1] Even so, it would be easy enough to write yet another macro that
does the terrain dereferencing.  This would cut down on duplicate code.

jason