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

["Raimar Falke" <rf13@xxxxxxxxxxxxxxxxx>]

On Mon, Feb 24, 2003 at 02:06:35AM -0800, Jason Short wrote:
> 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.

Sorry. I have forgot to add "But my point still stands."

> 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 rule is that all functions/macros take/return normal map
positions. The only exceptions are:
 - CHECK_MAP_POS
 - is_real_map_pos
 - is_normal_map_pos
 - regular_map_pos_is_normal

This is an easy rule. Everybody can remember it.

Why should this rule be broken for square_iterate? Why square_iterate
and not is_water_adjacent_to_tile. How should the user know that he
has to provide normal map positions to is_water_adjacent_to_tile but
not to square_iterate? There is no reason why square_iterate is
special.

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

I see the new for rectangle_iterate in the mapview. But if it is
created it doesn't use map positions. It just sets an arbitrary 2D
point to various positions. I would like to make this explicit like
non_map_rectangle_iterate but I guess a comment has to be enough.

> BTW: you reproduce the crash you need to compile with --enable-debug.

Doh! Obviously. Thanks.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  "Heuer's Law: Any feature is a bug unless it can be turned off."