[Freeciv-Dev] Re: [PATCH] Corecleanup_07Part2 has been put in incoming

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sat, Sep 01, 2001 at 05:41:06AM +0200, Gaute B Strokkenes wrote:
> On Thu, 30 Aug 2001, gs234@xxxxxxxxx wrote:
> 
> > When I get back from work I intend to change is_real_tile()
> > IS_VALID_POS(), on the grounds that it tests a coordinate pair
> > rather than a tile pointer, and because a position if valid iff it
> > refers to a tile on the map.  I will change normalize_map_pos() to
> > always adjust the x coordinate, and then I will add a IS_SANE_POS()
> > to test whether a position refers to a tile on the map and is in
> > canonical form.
> 
> And here it is.
> 
> In addition to the above, this patch converts a lot of manual loops to
> use iteration macros and fixes the odd bug here and there.  It also
> adds a function nearest_real_tile() which is the functional equivalent
> of map_adjust_x() and map_adjust_y() when applied to a position.
> There is another macro, MAPSTEP(), which makes a single step in a
> given direction and then normalizes the result.
> 
> I've also taken out the access changes to the map_[gs]et_* functions;
> while I think that they are desirable in general I do not think the
> codebase in general is quite ready for them yet.
> 
> I plan to apply this after I've given it some testing; there should be
> little here to offend anyone.
> 
> Note that the patch doesn't show is_real_tile -> IS_VALID_POS, since
> this would only clutter the patch.

There is a lot of noise (the "_end-}" thing, s/return(a)/return a/
thing, other formatting changes,...) in this patch. Is this necessary?

> -  }
> -  city_map_checked_iterate_end;
> +  } city_map_checked_iterate_end;

Although it is a bit ugly indent moves the *_end on a new line.

> -         ok++;
> -         break;              /* out of adjc_iterate */
> -       }
> +     if (map_get_terrain(x2, y2) == T_OCEAN
> +         && is_my_zoc(unit_owner(punit), x2, y2)) {
> +       ok++;
> +       goto OK;
> +
> +       /* FIXME: The above used to be "break; out of adjc_iterate",
> +       but this is incorrect since the current adjc_iterate() is
> +       implemented as two nested loops.  */

You fixed it. So the comment is useless.

>  static void find_city_beach( struct city *pc, struct unit *punit, int *x, 
> int *y)
> -  int search_dist = real_map_distance( pc->x, pc->y, punit->x, punit->y );
> +  int search_dist = real_map_distance(pc->x, pc->y, punit->x, punit->y) - 1;
> +  
> -  square_iterate(punit->x, punit->y, search_dist - 1, xx, yy) {
> +  square_iterate(punit->x, punit->y, search_dist, xx, yy) {

Well this is equivalent but why did you changed it?

> -  for (tt = T_FIRST; tt < T_COUNT; tt++) {
> -    if (0 == strcmp (tile_types[tt].terrain_name, name)) {
> +  for (tt = T_FIRST; tt < T_COUNT; tt++)
> +    if (!strcmp (tile_types[tt].terrain_name, name))

I like the old form more.

> -  if (type < T_COUNT) {
> -    return get_tile_type(type)->terrain_name;
> -  } else {
> -    return NULL;
> -  }
> +  assert(type < T_COUNT);
> +  return tile_types[type].terrain_name;

Have I mentioned that I like asserts?

> +/* We're using a trick here.  The straightforward way to write
> +IS_VALID_POS() would be as
> +
> +  0 <= (y) && (y) < map.ysize
> +
> +however, since we know that map.ysize is always positive and we know
> +that the bit-pattern of a negative signed int is the same as the
> +bit-pattern of a very large number when interpreted as an unsigned
> +int, we can do away with the extra check.  
> +*/
> +  
> +#define IS_VALID_POS(x,y) \
> +  (((unsigned) (y)) < ((unsigned) map.ysize))
> +
> +#define is_real_tile(x,y) IS_VALID_POS(x,y)
> +
> +#define IS_PROP_TILE(x,y)                          \
> +  (IS_VALID_POS((x), (y))                          \
> +   && (((unsigned) (y)) < ((unsigned) map.ysize))  )

I don't see a difference between IS_VALID_POS and IS_PROP_TILE.

> +#define MAPSTEP(x,y,x1,y1,dir)     \
> +  do {                                  \
> +    (x) = (x1) + DIR_DX[(dir)];         \
> +    (y) = (y1) + DIR_DY[(dir)];         \
> + /* if ((x) == -1)          */  \
> + /*   (x) = map.xsize-1;       */  \
> + /* else if ((x) == map.xsize) */  \
> + /*   (x) = 0;                      */  \
> +    (x) = map_adjust_x((x));       \
> +  } while (0)                         

IMHO MAPSTEP should be a method which does:
int mapstep(int old_x,int old_y,int *new_x,int *new_y,int dir)
{
        *new_x = old_x + DIR_DX[dir];
        *new_y = old_y + DIR_DY[dir];
        return normalize_map_pos(new_x,new_y);
}

With such a method we get everything which has to do with position
creating and checking in one method and place. The method resembles
common code structures.

Overall you should really remove the noise of the patch. I'm not sure
but it looks to me that some of loops you fixed are allready fixed in
the cvs.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  This customer comes into the computer store. "I'm looking for a mystery
  Adventure Game with lots of graphics. You know, something realy
  challenging". "Well," replied the clerk, "have you tried Windows 98 ?"