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

["rwetmore@xxxxxxxxxxxx" <rwetmore@xxxxxxxxxxxx>]

Raimar Falke wrote:
> On Wed, Feb 26, 2003 at 02:47:20AM -0800, Raimar Falke wrote:
> 
>>On Wed, Feb 26, 2003 at 02:28:17AM -0800, Per I. Mathisen wrote:
>>
>>>On Wed, 26 Feb 2003, Raimar Falke wrote:
[...]
>>>Writing a new iterator which is exactly like the square_iterate only that
>>>it doesn't have an assert is plain stupid. If the assert is that much of a
>>>bother, remove it. I'm with Ross and Jason on this one.

The only value of the assert is to break the original interface and
cause unnecessary core dumps, or force people to write their own code
to avoid this.

[...]
> Longer explanation:

Shows in depth that you don't understand :-).

> There are two types of users of square_iterate:

Possibly true - those that know how to use it and those that only
think they do :-).

>  - first type always passed normal map positions in. To check this we
>  test for this. If we avoid the check we won't notice the problem of
>  non-normal positions in the first. This is bad.

Non-normal center positions or even unreal center positions are not a
problem. The macro will still iterate and output the real/normalized
positions within the radius in its if-clause, and unreal ones in the else.

There is no need to test the inputs for the macro for this to function
correctly.

>  - second type may also pass non-real map positions in. These aren't
>  an error here.

They weren't an error for square_iterate in the first case either so
this is still true.

> Solutions:
>  - two macros: current and an unchecked_

You don't need two macros - there is no check needed for the macro to
function correctly in all cases.

You do need to revert the current bug, though.

>  - one macro without the assert. Then the assert has to move out to
>  the users of the macro. Programmers will forget to add the test -> bad

They don't need to add a test for the macro. Nothing bad except in
your mind.

If they need to test the coordinates for some other reason, this has
nothing to do with the macro and everything to do with the local code
involved at that point.

>  - convert the second of users of square_iterate to the first one: it
>  looks like this can be done.

It breaks the macro as has become very clear. It will force people to
write their own code instead of using the macro, which is the reason
for the macro in the first place.

> So I go for the first solution.

I go for fixing the current bug and otherwise not breaking anymore code
i.e. single (once more robust) macro.

>       Raimar

Square_iterate and all the other iterators have the same interface.

It will be interesting when Raimar has broken them all, everyone stops
using them in favor of rolling their own loops and FreeCIV goes back to
the days when map code was handled and mishandled differently everywhere
causing all sorts of problems.

Cheers,
RossW
=====