[Freeciv-Dev] Re: PATCH: check_map_pos

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:
> 
> On Mon, Oct 15, 2001 at 05:33:45AM -0400, Jason Dorje Short wrote:
> > Raimar Falke wrote:
> > >
> > > On Mon, Oct 15, 2001 at 04:29:10AM -0400, Jason Dorje Short wrote:
> > > > The attached patch introduces a check_map_pos() function.
> > > >
> > > > It's done as an inline function within map.h, but this is unimportant at
> > > > this point.  As said before, the advantage of a function is that it can
> > > > be used within map_inx if desired; there's not really any advantage to a
> > > > macro.
> > > >
> > > > It does not touch client code.  With all the different GUI's and the
> > > > tendancy to pass around coordinates picked by the user, the client is
> > > > not yet ready for this function.
> > > >
> > > > I have run several full (endyear=2000) autogames, and have not had any
> > > > problems yet.
> > > >
> > > > Note that check_map_pos is not called on coordinates that are simply
> > > > passed off to another function; for instance most of the code in map.c
> > > > just calls MAP_TILE or map_inx with the coordinates so it's not
> > > > necessary to call check_map_pos directly.
> > >
> > > So since the code base is ready for the change we have to discuss
> > > about the form.
> >
> > Unfortunately the client isn't ready for this change.  It will fail the
> > assertion after a little while.
> 
> The client is ready. The only problem I found was
> 
> client/control.c:
> 
> void do_move_unit(struct unit *punit, struct packet_unit_info *pinfo)
> {
>   int x, y, was_teleported;
> 
>   was_teleported=!is_tiles_adjacent(punit->x, punit->y, pinfo->x, pinfo->y);
>   x=punit->x;
>   y=punit->y;
> 
>   punit->x=-1;  /* focus hack - if we're moving the unit in focus, it wont
>                  * be redrawn on top of the city */
>   ^^^^^^^^^^^^
> 
> If you know any problem with the client please say so. We can fix them
> before the check_map_pos patch is applied.

This is the first problem found.  I do not know what this code is
intended to do, but since it's accessing a tile at an incorrect tile I'm
pretty sure it doesn't do it.  Can anyone shed light on this?

Even after removing this line, though, there are still problems.  The
one I'm looking at now is in fill_tile_sprite_array.

> > We can still make the change, if we remove the assert(0) in
> > check_map_pos.  That way there will be error messages, but no outright
> > problems.  It will also be easy enough to reinsert the assert() call to
> > get core files once problems are found.
> >
> > > IMHO we should use macros to be able to completely
> > > disable this checks. IMHO it isn't a good idea to mix these two things
> > > (introduction of check_map_pos and inline).
> >
> > Fine.  Only two things: the macro can not be used inside map_inx, and
> > map_inx is where the problems are most likely to occur.  Also, I'd
> > prefer to keep the name check_map_pos (instead of check_map_pos); the
> > macro can ensure that passed parameters are not evaluated more than
> > once.  This last will make it easier to convert to a function later if
> > desired.
> 
> IMHO if we touch this method again it should to remove it. If we agree
> that the basic access method will always do checks we can remove
> certain checks. Like in:
> 
> diff -u -r1.96 map.c
> --- common/map.c        2001/10/11 12:37:04     1.96
> +++ common/map.c        2001/10/15 08:26:52
> @@ -1068,8 +1068,7 @@
>    int maxcost = 72; /* should be big enough without being TOO big */
>    struct tile *tile0, *tile1;
> 
> -  assert(is_real_tile(x, y));
> -  normalize_map_pos(&x, &y);
> +  check_map_pos(&x, &y);
> 
>    tile0 = map_get_tile(x, y);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This will check.
> 
> In the good case the extra check in check_map_pos is unnecessary. So
> if each map position is at some time used to access map data via the
> basic access methods we can remove the check_map_pos checks
> altogether. The only extra gain is that the bad map positions are
> catched earlier in the call chain and it is a kind of documentation
> (that this method expects normal map positions).

Good point.  Note that there are also other arrays and such where such
accesses should be checked (for instance hmap()), but for the most part
these checks will be spurious and can be removed.  At that point we
should achieve a real gain in code complexity and (perhaps) performance.

jason