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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Oct 12, 2001 at 05:40:13AM -0400, Jason Dorje Short wrote:
> 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)).
> 
> ...
> 
> > 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.
> 
> Upon further reflection, I really like the following change that unifies
> check_coords with CHECK_MAP_POS.
> 
> Though we may want to rename the function check_map_pos().  It could
> also be made a macro, if desired.
> 
> 
> /**************************************************************************
> -...
> +This used to return an integer value on whether the position was real
> or
> +not.  However, its "new" use is basically to assert that the position
> is
> +normal.  If not, it will abort when in debug mode and fix the position
> +otherwise.  Thus, there is no longer a need for a return value, since
> the
> +coordinates will be usable either way.
> 
> **************************************************************************/
> -int check_coords(int *x, int *y)
> +void check_coords(int *x, int *y)
>  {
>    int save_x = *x, save_y = *y;
>  
> -  if (!normalize_map_pos(x, y)) {
> -    freelog(LOG_ERROR, "%d, %d is not a real tile!", *x, *y);
> +  if (!normalize_map_pos(x, y) || *x != save_x || *y != save_y) {
>      nearest_real_pos(x, y);
> -    return 0;
> +    freelog(LOG_ERROR, "(%d, %d) is not a proper position.  Adjusted to
> (%d, %d)!", save_x, save_y, *x, *y);
> +    assert(0);

+ freelog(LOG_ERROR, "If you can read this you have compiled without debug 
support. Please contact <freeciv-dev@@freeciv.org> and report this error").

>    }
> -
> -  if (*x != save_x || *y != save_y)
> -    freelog(LOG_ERROR, "Had to adjust coords %d, %d", *x, *y);
> -
> -  return 1;
>  }
> 
> (Though actually the code itself is unoptimized, but that's easily
> fixed.)

Rewrite it and change the name. However note that I'm still not sure
if this is the way to go. check_map_pos has a function call overhead
we don't want. So the first step in all cases is the introduction of
CHECK_MAP_POS. We can then argue how CHECK_MAP_POS should be
implemented and if it should segv or not.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  Microsoft does have a year 2000 problem. I'm part of it. I'm running Linux.