[Freeciv-Dev] Re: (PR#3450) square_iterate and unreal positions

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

Raimar Falke wrote:
> On Sun, Feb 23, 2003 at 03:25:25PM -0800, Jason Short wrote:
> 
>>[jdorje - Mon Feb 17 08:08:37 2003]:
>>
>>
>>>For some reason square_iterate calls CHECK_MAP_POS on its center tile. 
>>>This is a problem since it only shows up in debug mode and is not 
>>>obvious to the programmer.  And I really don't know why it does that (I 
>>>wrote the code for it, some time ago).
>>>
>>>This CHECK_MAP_POS should probably be removed.
>>
>>And the patch.
> 
> No. No. No. Just because one user has problems with the check you
> can't remove it and so punish the other users. Per fixed it in the
> correct way in 3484 by fixing this one user.

Huh?  Earlier you said:

Raimar Falke wrote:
 > For the crash you need iso mode. But even with this I can't reproduce
 > but I can see from the code why is crashes. You are right that x==0
 > triggered it. The problem is that
 >
 >      square_iterate(x - 1, y - 1, 1, city_x, city_y) {
 >                        ^^^^^^^^^^^^
 >        if ((pcity = map_get_city(city_x, city_y))) {
 >          get_canvas_xy(city_x, city_y, &canvas_x, &canvas_y);
 >          show_city_desc(pcity, canvas_x, canvas_y);
 >        }
 >      } square_iterate_end;
 >
 > will create unreal map positions and square_iterate wants real
 > ones. The code has to normalize the map position before it is passed
 > to square_iterate.
 >
 > Jason: I expect that you see this as a prove that we should remove the
 > check from square_iterate.

which I took to mean you now agreed that the CHECK_MAP_POS should go.

The problem is we can't 'fix' the caller, since the caller isn't broken 
in any way.  The broken thing here is square_iterate - it declares that 
the center position of the square _must_ be real, when there is no 
reason why this must be the case.  Since all other users do satisfy 
this, having this check in is slightly helpful in debugging - but it is 
still broken.

The alternative is to declare that square_dxy_iterate (and 
square_iterate, circle iterate, and other wrappers) must have a valid 
center, and introduce a new iterator macro for the new case.  I've 
already proposed rectangle_iterate, which is a more generally useful 
macro than square_iterate anyway.  But I think this macro needs to go 
into map.h - we don't want localized macros spread around all over the 
code; they should be all in one place.  This is a general macro, it's 
just that it only has two users right now (both in mapview_common.c).

BTW: you reproduce the crash you need to compile with --enable-debug. 
Then just play enough in iso-view, and you will inevitably trigger the 
assertion.

jason