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

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

Raimar Falke wrote:
> On Mon, Feb 24, 2003 at 04:01:33AM -0800, Jason Short wrote:
> 
>>Raimar Falke wrote:
>>
>>>On Mon, Feb 24, 2003 at 02:44:37AM -0800, Jason Short wrote:
[...]
> I want to catch errors (here non-normal map positions passed to
> square_iterate). I want to catch the case where in

square_iterate arguments do not need to be normalized. The code
is 100% robust and will never break without this religious
requirement. You should try really hard to understand this point.

In fact this applies to all the map iterators.

> bool is_friendly_city_near(struct player *owner, int x, int y)
> {
>   square_iterate (x, y, 3, x1, y1) {
> 
> the caller didn't provide a normal map position.

Then test the function arguments outside of the macro in this one
place only.

But note, it is perfectly safe to do this if all that is done
here is to execute square iterate and no other map accesses are
made directly with (x,y).

>   If a non-normal map position is passed this will go unnoticed.

square_iterate understands this and doesn't need to check its
arguments, only its outputs. Not noticing a non-problem is not a
problem.

You should learn to be flexible, and try to understand when
normalization is required and when it is not. Don't substitute
your own manufactured, foolishly simple rules as the requirement
for doing things. Learn, understand and apply the real arguments.

> This is bad. 

This is religion. It has nothing to do with correctness or
robustness which is all that should be of concern here..

Using the excuse that this is an Inquisition tool to stomp out a
heresy that is only a heresy in your religion is also not a good
argument. There is a certain circular dependency that should
suggest the inherent fallacy.

 > So the CHECK_MAP_POS was added to square_iterate. This
> isn't a problem since most of the users of square_iterate just pass
> some variable in. The only other user I caught with
> 'square_itera.*-[^>]' was

Clearly you didn't look far enough or think about the cases or
concepts you might have missed. Just because you didn't find anything
is not a reason to add a spurious restriction and code that guarantees
its use will be broken.

Adding the check was a *very* *bad* programming error. It changed
a correctly functionning and widely used macro into one that now
produces core dumps. I hope you feel an appropriate level of shame.

[...]
>       Raimar

Cheers,
RossW
=====