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

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:
> 
> 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).

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

OK, I'll take a deeper look.  I'm very uncomfortable with calling
normalize_map_pos and not checking the return value at all, which is
what this code currently does.

> > @@ -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?

(x0, y0) need not be normal since the iterator macro right below it will
take care of things.

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

The iterate macros themselves work fine with non-normal coordinates:
they just add on offsets and then normalize.  Only in this check is it
specifically required that a position be normal (otherwise
IS_BORDER_MAP_POS will always return 0).  I'd prefer to leave the
check_map_pos near to where the actual assumption is being made.

jason