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

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

On Mon, 29 Oct 2001, vze2zq63@xxxxxxxxxxx wrote:

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

Why does this follow?

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

I see you have changed IS_BORDER_MAP_POS() and map_inx() so that they
have to take lvalues as arguments.  What's the point?  As pointed out
elsewhere, you can not reliably repair this sort of damage in the
arbitrary topologies that you are proposing.

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

As has been mentioned a couple of times already, the idea is to find
bugs.  We are aiming for a model in which all functions should produce
/ expect normalised coordinates unless there is a very good reason why
not.  So if we pass unnormalised coordinates to a funtion / macro,
that's a bug; and we want to know about it.

If we were to take the approach that you seem to be advocating, which
is that it's ok to pass unnormalised coordinates to a function if you
know that it won't matter, then we would have to carefully check the
code everytime we make a change to make sure that it still will not
matter, or to see if we need to add a check (and also check all the
callers of that function or macro).  That will not make it any easier
to debug Freeciv.

I propose the following:

check_pos.diff
Description: Text document

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
Am I accompanied by a PARENT or GUARDIAN?