[Freeciv-Dev] Re: PATCH: load_map_data macro (PR#1012)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sat, Oct 13, 2001 at 04:36:08PM -0700, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
> The attached patch implements a LOAD_MAP_POS macro.  It then makes code
> replacement like replacing
> 
>   for(y=0; y<map.ysize; y++) {
>     char *terline=secfile_lookup_str(file, "map.t%03d", y);
>     for(x=0; x<map.xsize; x++) {
>       char *pch;
>       if(!(pch=strchr(terrain_chars, terline[x]))) {
>        freelog(LOG_FATAL, "unknown terrain type (map.t) in map "
>                "at position (%d,%d): %d '%c'", x, y, terline[x],
> terline[x]);
>        exit(1);
>       }
>       map_get_tile(x, y)->terrain=pch-terrain_chars;
>     }
>   }
> 
> with
> 
>   LOAD_MAP_DATA(secfile_lookup_str(file, "map.t%03d", y),
>                 map_get_tile(x, y)->terrain = char2terrain(ch));
> 
> There are three advantages to this new code: it leads to less
> duplication, it "fixes" code so that it is topology-independent
> (LOAD_MAP_POS skips over non-normal positions), and it introduces extra
> error-checking that was previously done in only a few of the loops.
> 
> The disadvantage of the code is that it might introduce a bug.  See the
> following comment about the macro itself:
> 
> /* Note: some of the code this is replacing used to skip over lines that
>    did not exist.  I don't know whether this was for backwards-
>    compatibility or just because the loops were written differently.
>    This macro will fail if any line does not exist.  Hopefully this will
>    never happen, but if so we at least show an informative message. */
> 
> After some thought, I don't think we'll easily be able to find out if
> the line-skipping is needed for backwards-compatibility.  If so, then
> that backwards-compitibility will no longer be present; but since this
> code is at least a year old I think the damage should be small.  If not,
> then we've saved some extra ugliness in the code.  This change should
> have no effect on current savegames (unless there's another bug in the
> code that causes data to not be saved correctly).
> 
> It is difficult to verify the correctness of the patch.  Unlike that
> SAVE_MAP_DATA patch, we can't just save a game and make sure it's
> identical.  This code looks correct to me (I'll double-check it again
> soon) and has given correct results in all my tests.  Having more eyes
> on it would definitely be good.
> 
> This patch leaves the map_rivers_overlay_load function intact.  The
> original SAVE_MAP_DATA removed (IIRC) map_rivers_overlay_save.  Although
> the load function could easily be removed in the same way, a comment in
> the code indicates that the function may need to be called more than
> once (although it is not currently).  So, I could go either way on this.
> 
> The patch also introduces hex2dec() and char2terrain() functions to
> convert from the character value in the savefile to the decimal or
> terrain integer value.  I have made these functions, since they do a
> fair amount of error-checking and the branching would have been quite
> ugly as a macro.  I've also put them up alongside DEC2HEX, dec2hex[],
> and terrain_chars[]; this means they are above the [LOAD|SAVE]_MAP_POS
> macros.  They can easily be moved elsewhere (further down with the other
> functions) if desired.

> +static const char dec2hex[] = "0123456789abcdef";

hex2dec and dec2hex may not be the best names. What about print_hex
and scan_hex (parse_hex)? These method are just short and much more
faster versions of sprintf and sscanf.

> +    LOAD_MAP_DATA(secfile_lookup_str(file, "map.a%03d", y),
> +               map_get_tile(x, y)->known |= hex2dec(ch, 0));
                                            ^ wrong
> +    LOAD_MAP_DATA(secfile_lookup_str(file, "map.b%03d", y),
> +               map_get_tile(x, y)->known |= hex2dec(ch, 1));
> +    LOAD_MAP_DATA(secfile_lookup_str(file, "map.c%03d", y),
> +               map_get_tile(x, y)->known |= hex2dec(ch, 2));
> +    LOAD_MAP_DATA(secfile_lookup_str(file, "map.d%03d", y),
> +               map_get_tile(x, y)->known |= hex2dec(ch, 3));


> +      /* get 4-bit segments of the second half of the 32-bit "known" field */
> +      LOAD_MAP_DATA(secfile_lookup_str(file, "map.e%03d", y),
> +                 map_get_tile(x, y)->known |= hex2dec(ch, 4));
                                              ^ wrong
> +      LOAD_MAP_DATA(secfile_lookup_str(file, "map.g%03d", y),
> +                 map_get_tile(x, y)->known |= hex2dec(ch, 5));
> +      LOAD_MAP_DATA(secfile_lookup_str(file, "map.h%03d", y),
> +                 map_get_tile(x, y)->known |= hex2dec(ch, 6));
> +      LOAD_MAP_DATA(secfile_lookup_str(file, "map.i%03d", y),
> +                 map_get_tile(x, y)->known |= hex2dec(ch, 7));

Overall it looks like expected.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "There are three ways to get something done. Do it yourself, hire someone
  to do it for you or forbid your kids to do it."