[Freeciv-Dev] Re: Remove map_adjust_[xy] invocations from server (PR#1003)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Oct 12, 2001 at 05:18:57AM -0400, Jason Dorje Short wrote:
> Raimar Falke wrote:
> > 
> 
> [snip: uses of is_real_tile, normalize_map_pos, etc.]
> 
> > I plan to propose something like this:
> >  - there is a CHECK_MAP_POS(x,y) == assert(is_normal_map_pos(x,y));
> >  - every "assert(is_real_tile(x,y));normalize_map_pos(x,y)" is
> >  replaced by CHECK_MAP_POS
> >  - there is an extra check (if(!is_normal_map_pos(x,y)){y=x/0;})
> >  introduced in map_inx
> > 
> > In the short/medium term we can disable (but not remove)
> > CHECK_MAP_POS. We will probably leave the check in map_inx forever. So
> > I dislike the nearest_real_pos call above. If there are wrong map
> > positions we catch them (earlier(CHECK_MAP_POS) or later(map_inx)).
> 
> Most of this sounds reasonable.  However, two points:
> 
> 1.  I do not see the logic of forcing a crash when not in debugging
> mode.  Presumably people playing the game in this stage would appreciate
> it if we did everything possible to avoid crashes.  For the "extra
> check", why not have something like this:
> 
> if(!is_normal_map_pos(x, y)) {
>   assert(0);
>   freelog(...);
>   nearest_real_pos(&x, &y);
> }
> 
> This would give many fewer segfaults (namely: none) for end-user players
> of the game.  I'd use this check in place of every CHECK_MAP_POS.  In
> fact, this is almost identical to the current check_coords function
> (except that there's no assert() there; one could easily be added).
> 
> Since nearest_real_pos really is needed for the client (in several
> places, all related to moving the view around IIRC), we might as well
> use it in this situation.  There's really no way we can do without it
> without changing gameplay significantly.

If after some (no I don't say extensive here) testing from the
developers the released version contains some bugs are we want to
catch them. The saves way to get a bug report is an
assert/if(fail_cond){b=a/0;}. This is my attitude to this. The offical
freeciv one may differ. Thue?

> 2.  None of this is a reason not to go ahead with this patch.  This
> replaces one set of imperfect code with another set, but at least the
> new set uses the desired code constructs (i.e. normalize_map_pos instead
> of map_adjust_x()).

I know. I will apply your patch after some discussion time (read
tomorrow).

> Either way, I'm willing to come up with a patch that makes the checks we
> decide on (although fixing the code to use them everywhere will take
> more work).  Let me know what you want.

Feel free to code the CHECK_MAP_POS patch. I was also thinking about a

/*
 * 0 means no
 * 1 means minimal checks (in map_inx)
 * 2 means full checks (CHECK_MAP_POS actived)
 */
#define MAP_CHECKS 2

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Of course, someone who knows more about this will correct me if I'm
  wrong, and someone who knows less will correct me if I'm right."
    -- David Palmer (palmer@xxxxxxxxxxxxxxxxxx)