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

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

Ahh.  I thought you meant you expected me to come around to your point 
of view, which was that CHECK_MAP_POS should go.  I thought this odd, 
since that was my point of view not yours :-).

And yes, I do see this as evidence that the CHECK_MAP_POS should go. 
But it's not _new_ evidence; this was my original reason.

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

square_iterate is special because it operates over a range of map 
positions.  Why is it that it is the center position of the square that 
must be normal, and not, say, the bottom right position?

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

You're wrong: the iterate loops in question are iterating over map 
positions, not GUI positions.  The code it uses is completely identical 
to what square_iterate uses, only without the normal-center requirement.

There is a need for GUI position iterators, though - in 
show_city_descriptions and update_map_canvas, for instance.  But this is 
not an easy concept, and there aren't enough users (yet) to justify me 
trying to write them...

>>BTW: you reproduce the crash you need to compile with --enable-debug.
> 
> Doh! Obviously. Thanks.

Just as obviously, I can't type (s/you/to/)...

jason