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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

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)?

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "- Amiga Y2K fixes (a bit late, wouldn't you say?)"
    -- Linus Torvalds about linux 2.4.0 at 4 Jan 2001