[Freeciv-Dev] Re: PATCH: check_map_pos

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

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.

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

> Perhaps an intermediate step would introduce check_map_pos as a
> non-inline function?  This would give a performance hit, but otherwise I
> think we're going to have some ugliness here.
> 
> In the meantime I can try to track down the client problems, although
> they may be GUI-specific.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "#!/usr/bin/perl -w
  if ( `date +%w` != 1 ) {
    die "This script only works on Mondays." ;
  }"
    -- from chkars.pl by Cornelius Krasel in de.comp.os.linux.misc