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

["Raimar Falke" <rf13@xxxxxxxxxxxxxxxxx>]

On Mon, Feb 24, 2003 at 05:09:25PM -0800, rwetmore@xxxxxxxxxxxx wrote:
> 
> 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.

Only if the following holds true:
 n(f(n(x))) == n(f(x))

n = normalize
f = a function which changed the position. Adding (-1, -1) for example.

I have no idea if this holds true or not. In the current code or in
all future possibilities.

> The code is 100% robust

I agree that the code is robust. I think it is too robust. Because of
the robustness it will conceal errors in other parts of the code.

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

That is one solution. It has the drawback that the programmer of the
common use has to do extra coding.

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

What outputs will square_iterate generate if you pass a non-real
position in? It depends on the map position you passed in. 

(-1, -1) will generate
 (-2, -2) non-real
 (-2, -1) non-real
 (-2,  0) non-normal

 (-1, -2) non-real
 (-1, -1) non-real
 (-1,  0) non-normal

 ( 0, -2) non-real
 ( 0, -1) non-real
 ( 0,  0) normal

(-10, -10) will generate only non-real positions.

So I argue that if you use square_iterate on a non-real map position
you get random behavior. You may now argue that this randomness has a
pattern and that square_iterate was coded to produce this pattern.

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

Ok. I rephrase this to "square_iterate should be able to catch errors
which are made by others parts of the code and not conceal the
errors."

> 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

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  One nuclear bomb can ruin your whole day.