[Freeciv-Dev] Re: [PATCH] Map cleanups (PR#1208)

[Jason Short <vze2zq63@xxxxxxxxxxx>]

Ross W. Wetmore wrote:

> I actually like something like this better than nearest_real_pos(). I am
> assuming when it is expressed correctly it effectively returns the original
> unstepped coordinates. More importantly, it should clearly document that 
> this is what it does as part of the interface.
Yes, as you've noticed this code is wrong (I'll call it a typo and leave 
it at that :-).  And it is certainly logically better (and in 
tilespec.c, more correct) than using nearest_real_pos.

> The case where one steps in a loop until move points are exhausted, and
> hits an unreal tile along the way (causing a serious slowdown in game
> response :-) is at least indirectly covered by this warning/documentation
> then.
Such code would be broken in more ways than one, but yes.


> At 05:23 AM 02/01/07 -0500, Jason Short wrote:
>
> > jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
> >
> >
> >
> > > I do think SAFE_MAPSTEP is wrongly implemented.  It needn't use 
> > > nearest_real_pos but should instead do
> > >  #define SAFE_MAPSTEP(x, y)
> > >  {
> > >    int _x = x, _y = y;
> > >    if (!normalize_map_pos(&x, &y)) {
> > >      x = _x;
> > >      y = _y;
> > >    }
> > >  }
> But if you look closely ...
>
> In fact what you have done here is one of the many local variants of
> normalize_map_pos() that are going to spring up because you didn't 
> make the condition we all say is desired, i.e. the one used here, part
> of the interface, and thus force people to implement this privately all 
> over the codebase.
Note quite...

First, this code is wrong, and the correct code does not use the 
construct you're talking about - it does MAPSTEP and reverts to the 
_original_ (unstepped) coordinates if the STEP fails.  So this is 
instead an example of one of the very few local variants that are going 
to spring up because we don't implement normalize_map_pos to use the 
condition none of us desire, i.e. returning the nearest_real_pos instead 
of the original coordinates.

But to address the real issue, I will repeat that almost all code that 
uses the structure I erronously typed above - i.e. calling 
normalize_map_pos, and then using the untouched coordinates if it 
returns FALSE (or saving the coordinates if normalize_map_pos does not 
guarantee this) is wrong/bad code.
I've already shown one example of "saved" values.  In that example it 
was necessary to save the values even for the real case (where normalize 
return TRUE), since it was the absolute positions that were needed in 
both cases.  To generalize this, all unreal coordinates are equal; they 
are all unreal and useless for most or all calculations.  In almost all 
cases, when we do need to do coordinate handling in the unreal case it's 
because we need to work with the _absolute_ coordinates rather than with 
the normalized coordinates.  In such cases, we _have_ to store the 
absolute coordinates, and normalize the map coordinates separately.  It 
may be tempting to use the normalized coordinate in the real case and 
the absolute coordinates in the unreal case, but this is usually wrong.
For a real-world example, here is an example of code that you might 
think would benefit from what you are suggesting, but in fact does not: 
http://www.freeciv.org/lxr/source/client/gui-gtk/mapview.c?v=cvs#L1130
I've looked through all current freeciv code to try to find an example 
that uses the coordinates after normalize_map_pos returns FALSE (so that 
I could explain how it was wrong :-), but I cannot find one.  There may 
be such a usage where it happens far from the actual invocation of 
normalize_map_pos, but it appears that all current code is correct in 
this manner, and introducing this into the definition of 
normalize_map_pos will not safe us anything.

> It is better to just do it. Think of it as a code reduction measure, 
> i.e. of all the duplicate code you are saving by moving it into the
> implementation from just outside :-).
Saving duplicate code is most useful when it is more likely to prevent 
bugs than to cause them.

> If someone really wants arbitrary values returned, and the void_tile
> sentiments revive to carry the day to give this a new lease on life, 
> then it is possible to expose the underlying internal interface in 
> normalize_map_pos(), i.e. below the is_real check and let them 
> experience the Chinese curse of "may you live in interesting times".
But while we're on the topic, here is an aside: there are a growing 
number of calls of the form:
  is_real = normalize_map_pos(&x, &y);
  assert(is_real);

in such cases, having a non-real position means a serious bug, and 
deserves the assertion.  But if we compile with NDEBUG we'll just 
continue merrily on our way, which probably not a good idea (it'll lead 
to segfaults and very unpredictible behavior).  It would be much safer 
to accept the void-tilish fix and do something like
  {
    int _x = x, _y = y;
    if (!normalize_map_pos(&x, &y)) {
      assert(0);
      x = _x, y = _y;
      nearest_real_pos(&x, &y);
    }
  }
In fact, this is the only valid piece of code I can think of that uses 
the (x,y) values of normalize_map_pos after it's returned FALSE. 
Although it will certainly not guarantee any kind of correct behavior, 
it will generally give many fewer segfaults and less unpredictable code 
than without the nearest_real_pos call.  This _could_ also be written as
  if (!nearest_real_pos(&x, &y)) {
    assert(0);
  }

if we change that function as you suggest.


In any case, there are currently 32 calls to normalize_map_pos that 
assume the position is real.  Most use the first construct up above, but 
a few use slower or even incorrect constructs instead (also, many of 
them should use CHECK_MAP_POS).  I think a new macro, 
normalize_real_map_pos would be appropriate to deal with this.

And, on a similar note, there are a number of places that do a similar 
thing with MAPSTEP; i.e. asserting the return value is real.  Perhaps a 
similar macro should be made up for them.  Unfortunately, the best name 
for it is SAFE_MAPSTEP and that is (unfortunately) already taken.
Now, back to the discussion...


> This way at least code that uses the primary interface won't suffer.
> And you will have to go slightly out of your way to do the stupid
> thing. It is always wise to make the defaults safe, and not the 
> "special" case :-).
I have exactly the same sentiments.  Using the (x,y) values returned by 
normalize_map_pos after it returns FALSE is almost always the stupid 
thing.  So again, I'm not saying normalize_map_pos shouldn't leave the 
values untouched - I'm just saying the interface should specify 
undefined values in this case, so that developers will have to go out of 
their way to use this dangerous construct.

jason