[Freeciv-Dev] Re: PATCH: check_map_pos

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Oct 15, 2001 at 06:29:09AM -0400, Jason Dorje Short wrote:
> 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()),

Yes and we catch all these accesses with map_inx.

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

So the question is: do we introduce check_map_pos/CHECK_MAP_POS or
just add checks to map_inx?

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 Windows: Where do you want to go today?
 Linux: Where do you want to go tomorrow?
 BSD: Are you guys coming or what?