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

[Jason Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:

> On Tue, Jan 08, 2002 at 03:41:24PM -0500, Jason Short wrote:
>
> > Raimar Falke wrote:
> >
> > > On Tue, Jan 08, 2002 at 03:50:59AM -0500, Jason Short wrote:
> > >
> > >>Ross W. Wetmore wrote:
> >
> > >>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/
> >
> > You'll still have to cache the values.  It's not the pre-normalization
> > positions we need, it's the pre-MAPSTEPping ones.  So the only way to
> > unify these macros is if normalize_map_pos returns the nearest_real_pos
> > coordinates.
> >
> > My miswriting of SAFE_MAPSTEP in the previous mail is probably confusing
> > you.  The correct way to write it is
> >
> > #define SAFE_MAPSTEP(dest_x, dest_y, src_x, src_y, dir) \
> >     do {                                                 \
> >       DIRSTEP(dest_x, dest_y, dir);                     \
> >       (dest_x) += (src_x);                              \
> >       (dest_y) += (src_y);                              \
> >       if (!normalize_map_pos(&(dest_x), &(dest_y))) {    \
> >         (dest_x) = (src_x);                              \
> >         (dest_y) = (src_y);                              \
> >         CHECK_MAP_POS((dest_x), (dest_y));               \
> >       }                                                  \
> >     } while(0)
> You are right.
>
>
> > >>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.
> >
> > Well, if we're not sure the coordinates are normal (as we're not) then
> > we need to do the normalization for both DEBUG and NDEBUG.  Substituting
> > CHECK_MAP_POS in for these will allow us to gain speed (if we choose) in
> > the NDEBUG case.
> What do you want the sample construct to be exchanged with?
>
>  CHECK_MAP_POS(x,y);
>  normalize_map_pos(&x, &y);
> or
>  normalize_map_pos(&x, &y);
>  CHECK_MAP_POS(x,y);
> or
>  CHECK_MAP_POS(x,y);
>
> The first two makes the non-NDEBUG case slower. And the last is a more
> stricter check than the sample construct.
>

> > >>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?
> >
> > I'm not concerned about the optimization, but about consistency,
> > readability, and correctness.  Making this a macro saves about 32x2
> > lines of code overall, and unifies the structures into one
> > sure-to-be-correct one.
> So you suggest something like: rename CHECK_MAP_POS to
> CHECK_FOR_NORMAL_MAP_POS, add CHECK_FOR_REAL_MAP_POS and
> CHECK_FOR_REAL_MAP_POS has the nice side-effect of making the map
> position normal if this is possible (real)?
I think CHECK_MAP_POS should be left as-is (at least in name), and a new 
function/macro normalize_real_map_pos introduced.  Note the cases in 
which these are used: CHECK_MAP_POS is used when we fully expect the 
position to be normal, and usually can't deal at all with it being 
unreal.  normalize_real_map_pos is used when we need a normalization but 
expect the position to be real (and can't or don't want to deal with the 
unreal case).
CHECK_MAP_POS therefore has to choose between speed and safety - in the 
NDEBUG case, it may choose to do the normalization (safety) or not 
(speed).  Currently it goes for speed.
normalize_real_map_pos _must_ do a normalization, so there's no real 
choice there.  The only choice is whether we do a nearest_real_pos in 
the case of unreal positions, but this is just a hack to avoid a 
potential crash.
Both should do full debugging checks; i.e. assertions that the expected 
conditions are met.
Now, you might wonder why normalize_real_map_pos is necessary at all. 
In a perfect world, it might not be, but freeciv has a number of places 
that pass around, for instance, goto directions for units.  Here we have 
the unit's original position and a direction.  We know the direction is 
valid, but we still must do the normalization anyway.
jason