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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Jan 08, 2002 at 03:50:59AM -0500, Jason Short wrote:
> 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.

The SAFE_MAPSTEP macro. You can do a s/SAFE_MAPSTEP/MAPSTEP/

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

Setting NDEBUG in the above case should gain speed. However you still
do the test in your proposed code. So I'm more for a new assert macro
which can't be disabled.

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

This is a optimization-only thing? How much percent of the runtime is
used for normalize_map_pos if CHECK_MAP_POS is disabled?

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

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Make it idiot-proof and someone will make a better idiot."