[Freeciv-Dev] Re: [PATCH] check_map_pos change (PR#1031)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Oct 28, 2001 at 05:00:54PM -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
> Raimar Falke wrote:
> > 
> > On Sun, Oct 28, 2001 at 02:58:20PM -0500, Jason Dorje Short wrote:
> > > Raimar Falke wrote:
> > > >
> > > > On Fri, Oct 26, 2001 at 01:10:17AM -0700, jdorje@xxxxxxxxxxxxxxxxxxxxx 
> > > > wrote:
> > > > > The attached patch implements and uses a macro called 
> > > > > check_map_pos(&x,
> > > > > &y) to check the coordinates (x,y) for normalness.
> > >
> > > > I think that we agreed that instead of replacing normalize_map_pos,
> > > > is_real_tile and co with check_map_pos we just remove them and trust
> > > > that we would catch all bad position in an access method (one which
> > > > uses map_inx). Look at this problem from this point of view: now every
> > > > method only accepts normal map positions. Why should some method have
> > > > a check for this and some not. Because of historical reasons?
> > > > No. Either all method have such a check or none. And I agree that none
> > > > is ok. If there are problems we can add extra checks. But we don't
> > > > expect problems ;)
> > >
> > > Yes, but I thought we were going to go with all-out replacement first
> > > and then scale back.
> > >
> > > No matter, I can remove the spurious checks.  I'd prefer to only remove
> > > the obviously spurious ones, though, and leave any that might be
> > > debatable until it's been in CVS for a while.
> > 
> > We will see how many checks are in your next patch.
> 
> I'm strongly opposed to the just-one-check-in-map-inx method of doing
> this.  If there is a bug, we will never find it.  Having too many
> check_map_pos calls is minor compared to this.
> 
> This patch has removed some of the check_map_pos calls (as many as I
> felt safe removing).
> 
> > > There really hasn't been enough testing of this to be even
> > > reasonably sure things are correct otherwise, and if we don't catch
> > > any mistakes in check_map_pos the odds are we won't catch them.
> > 
> > Note that before I accept this patch some extra checks have to be
> > inserted into map_inx.
> 
> IMO check_map_pos is check_map_pos is check_map_pos - the problems with
> having a failure at one place are the same as having them at another. 
> map_inx is one place where a check needs to be done, but I don't think
> it's more special than the others.  What extra check would you want?

I oversaw that you have added a check for map_inx.

> diff -u -r1.196 packhand.c
> --- client/packhand.c 2001/10/18 16:45:31     1.196
> +++ client/packhand.c 2001/10/29 00:53:52
> @@ -437,7 +437,7 @@
>  static void handle_city_packet_common(struct city *pcity, int is_new,
>                                        int popup, int investigate)
>  {
> -  int i;
> +  int i, is_real;
>  
>    if(is_new) {
>      unit_list_init(&pcity->units_supported);
> @@ -463,7 +463,8 @@
>      int d = (2 * r) + 1;
>      int x = pcity->x - r;
>      int y = pcity->y - r;
> -    normalize_map_pos(&x, &y);
> +    is_real = normalize_map_pos(&x, &y);
> +    assert(is_real);

I think this will crash. IMHO the calculations may result in unreal
map positions.

> @@ -1128,6 +1125,7 @@
>  ***************************************************************/
>  int is_tiles_adjacent(int x0, int y0, int x1, int y1)
>  {
> +  check_map_pos(&x1, &y1);

Why not check x0,y0?

>  #define IS_BORDER_MAP_POS(x, y)              \
> -  ((y) == 0 || (x) == 0 ||                   \
> +  (check_map_pos(&x, &y),                    \
> +   (y) == 0 || (x) == 0 ||                   \
>     (y) == map.ysize-1 || (x) == map.xsize-1)

??? Either do this check in all iterate macros or in none.

> @@ -452,7 +461,6 @@
>    int x_itr, y_itr, dir_itr, MACRO_border;                                   
>  \
>    int MACRO_center_x = (center_x);                                           
>  \
>    int MACRO_center_y = (center_y);                                           
>  \
> -  assert(is_normal_map_pos(MACRO_center_x, MACRO_center_y));                 
>  \
>    MACRO_border = IS_BORDER_MAP_POS(MACRO_center_x, MACRO_center_y);          
>  \
>    for (dir_itr = 0; dir_itr < 8; dir_itr++) {                                
>  \
>      DIRSTEP(x_itr, y_itr, dir_itr);                                          
>  \

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "From what I am reading Win98 and NT5.0 will be getting rid of all that
  crap anyway. Seems that Microsoft has invented something called TCP/IP and
  another really revolutionary concept called DNS that eliminates the
  netbios crap too. All that arping from browsers is going to go away.
  I also hear rumors that they are on the verge of breakthrough discoveries
  called NFS, and LPD too. Given enough time and money, they might
  eventually invent Unix."
    -- George Bonser in linux-kernel