[Freeciv-Dev] Re: [Patch] Special testing

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sat, Feb 16, 2002 at 07:12:29PM +0100, Petr Baudis wrote:
> Dear diary, on Sat, Feb 16, 2002 at 05:24:33PM CET, I got a letter,
> where Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> told me, that...
> > And another version. There are now map_has_special, tile_has_special
> > and contains_special.
> 
> Very nice! :)
> 
> Some objections, but not very serious (except overuse of contains_special() at
> some places and the S_ALL issue).
> 
> > Index: client/gui-gtk/mapview.c
> > ===================================================================
> > RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk/mapview.c,v
> > retrieving revision 1.117
> > diff -u -r1.117 mapview.c
> > --- client/gui-gtk/mapview.c        2002/02/12 08:42:23     1.117
> > +++ client/gui-gtk/mapview.c        2002/02/16 16:19:06
> > @@ -2239,19 +2239,19 @@
> >                      offset_x, offset_y_unit,
> >                      width, height_unit, fog);
> >    }
> > -  if (BOOL_VAL(special & S_AIRBASE) && draw_fortress_airbase)
> > +  if (contains_special(special, S_AIRBASE) && draw_fortress_airbase)
> >      pixmap_put_overlay_tile_draw(pm,
> >                              canvas_x, canvas_y-NORMAL_TILE_HEIGHT/2,
> >                              sprites.tx.airbase,
> >                              offset_x, offset_y_unit,
> >                              width, height_unit, fog);
> > -  if (BOOL_VAL(special & S_FALLOUT) && draw_pollution)
> > +  if (contains_special(special, S_FALLOUT) && draw_pollution)
> >      pixmap_put_overlay_tile_draw(pm,
> >                              canvas_x, canvas_y,
> >                              sprites.tx.fallout,
> >                              offset_x, offset_y,
> >                              width, height, fog);
> > -  if (BOOL_VAL(special & S_POLLUTION) && draw_pollution)
> > +  if (contains_special(special, S_POLLUTION) && draw_pollution)
> >      pixmap_put_overlay_tile_draw(pm,
> >                              canvas_x, canvas_y,
> >                              sprites.tx.pollution,
> > @@ -2287,7 +2287,7 @@
> >                                width, height_unit, fog);
> >    }
> >  
> > -  if (BOOL_VAL(special & S_FORTRESS) && draw_fortress_airbase)
> > +  if (contains_special(special, S_FORTRESS) && draw_fortress_airbase)
> >      pixmap_put_overlay_tile_draw(pm,
> >                              canvas_x, canvas_y-NORMAL_TILE_HEIGHT/2,
> >                              sprites.tx.fortress,
> 
> As the variable 'special' is used here only in these ifs, we can as well
> use map_has_special ;-).

But this will require extra lookups. It is also in a hot-path: the
drawing code.

> > Index: client/gui-mui/graphics.c
> > ===================================================================
> > RCS file: /home/freeciv/CVS/freeciv/client/gui-mui/graphics.c,v
> > retrieving revision 1.20
> > diff -u -r1.20 graphics.c
> > --- client/gui-mui/graphics.c       2002/02/12 08:42:24     1.20
> > +++ client/gui-mui/graphics.c       2002/02/16 16:19:08
> > @@ -1381,19 +1381,19 @@
> >               offset_x, offset_y_unit,
> >               width, height_unit, fog);
> >    }
> > -  if (BOOL_VAL(special & S_AIRBASE) && draw_fortress_airbase)
> > +  if (contains_special(special, S_AIRBASE) && draw_fortress_airbase)
> >      put_overlay_tile_draw(rp,
> >                       canvas_x, canvas_y-NORMAL_TILE_HEIGHT/2,
> >                       sprites.tx.airbase,
> >                       offset_x, offset_y_unit,
> >                       width, height_unit, fog);
> > -  if (BOOL_VAL(special & S_FALLOUT) && draw_pollution)
> > +  if (contains_special(special, S_FALLOUT) && draw_pollution)
> >      put_overlay_tile_draw(rp,
> >                       canvas_x, canvas_y,
> >                       sprites.tx.fallout,
> >                       offset_x, offset_y,
> >                       width, height, fog);
> > -  if (BOOL_VAL(special & S_POLLUTION) && draw_pollution)
> > +  if (contains_special(special, S_POLLUTION) && draw_pollution)
> >      put_overlay_tile_draw(rp,
> >                       canvas_x, canvas_y,
> >                       sprites.tx.pollution,
> > @@ -1429,7 +1429,7 @@
> >                         width, height_unit, fog);
> >    }
> >  
> > -  if (BOOL_VAL(special & S_FORTRESS) && draw_fortress_airbase)
> > +  if (contains_special(special, S_FORTRESS) && draw_fortress_airbase)
> >      put_overlay_tile_draw(rp,
> >                       canvas_x, canvas_y-NORMAL_TILE_HEIGHT/2,
> >                       sprites.tx.fortress,
> 
> I didn't check but it looks to me it should be same (Yet Another Case Of
> Complete Code Duplication? :).

Quite possible. Should be folded into mapview_common.

> > Index: common/city.c
> > ===================================================================
> > RCS file: /home/freeciv/CVS/freeciv/common/city.c,v
> > retrieving revision 1.144
> > diff -u -r1.144 city.c
> > --- common/city.c   2002/02/14 15:17:08     1.144
> > +++ common/city.c   2002/02/16 16:19:18
> > @@ -517,20 +517,20 @@
> >    enum tile_special_type spec_t = map_get_special(x, y);
> >    enum tile_terrain_type tile_t = map_get_terrain(x, y);
> >  
> > -  if (BOOL_VAL(spec_t & S_SPECIAL_1))
> > +  if (contains_special(spec_t, S_SPECIAL_1))
> >      s = get_tile_type(tile_t)->shield_special_1;
> > -  else if (BOOL_VAL(spec_t & S_SPECIAL_2))
> > +  else if (contains_special(spec_t, S_SPECIAL_2))
> >      s = get_tile_type(tile_t)->shield_special_2;
> >    else
> >      s = get_tile_type(tile_t)->shield;
> >  
> > -  if (BOOL_VAL(spec_t & S_MINE))
> > +  if (contains_special(spec_t, S_MINE))
> >      s += (get_tile_type(tile_t))->mining_shield_incr;
> > -  if (BOOL_VAL(spec_t & S_RAILROAD))
> > +  if (contains_special(spec_t, S_RAILROAD))
> >      s+=(s*terrain_control.rail_shield_bonus)/100;
> > -  if (BOOL_VAL(spec_t & S_POLLUTION))
> > +  if (contains_special(spec_t, S_POLLUTION))
> >      s-=(s*terrain_control.pollution_shield_penalty)/100; /* The shields 
> > here are icky */
> > -  if (BOOL_VAL(spec_t & S_FALLOUT))
> > +  if (contains_special(spec_t, S_FALLOUT))
> >      s-=(s*terrain_control.fallout_shield_penalty)/100;
> >    return s;
> >  }
> ..and the rest of diffs in this file..
> 
> As apparent from the context, you can use map_has_special() here as well.

Same as above.

> > Index: common/map.c
> > ===================================================================
> > RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
> > retrieving revision 1.112
> > diff -u -r1.112 map.c
> > --- common/map.c    2002/02/14 15:17:14     1.112
> > +++ common/map.c    2002/02/16 16:19:19
> > @@ -480,29 +480,29 @@
> >      /* range 0 .. 5 , 2 standard */
> >  
> >    case T_FOREST:
> > -    if (map_get_tile(x, y)->special) return 5;
> > +    if (map_has_special(x, y, S_ALL)) return 5;
> >      return 3;
> ..snip..
> 
> Will this work?

No. My fault.

> In fact, I can't imagine how this would.. The same issue as with the
> binary or, IMHO. We mut either add a special case for S_ALL in
> map_has_special(), introduce another function for this
> (map_has_any_special()), or remove the assert etc from the
> map_has_special and allow testing for more specials at once (which
> breaks encapsulation again).

Or change it to what was originally intended: has_special(S_SPECIAL1)
|| has_special(S_SPECIAL2).

> > @@ -1187,10 +1189,30 @@
> >  /***************************************************************
> >   Returns TRUE iff the given special is found at the given map
> >   position.
> > +***************************************************************/
> > +bool map_has_special(int x, int y, enum tile_special_type special)
> > +{
> > +  return contains_special(MAP_TILE(x, y)->special, special);
> > +}
> > +
> > +/***************************************************************
> > + Returns TRUE iff the given tile has the given special.
> > +***************************************************************/
> > +bool tile_has_special(struct tile *ptile, enum tile_special_type special)
> > +{
> > +  return contains_special(ptile->special, special);
> > +}
> > +
> > +/***************************************************************
> > + Returns TRUE iff the given special is found in the given set.
> >  ***************************************************************/
> > -int map_has_special(int x, int y, enum tile_special_type special)
> > +bool contains_special(enum tile_special_type set,
> > +                enum tile_special_type to_test_for)
> >  {
> > -  return BOOL_VAL(MAP_TILE(x, y)->special & special);
> > +  enum tile_special_type masked = set & to_test_for;
> > +
> > +  assert(masked == S_NO_SPECIAL || masked == to_test_for);
> > +  return masked == to_test_for;

> Is the S_NO_SPECIAL worth it? IMHO using plain zero is more clear and even 
> more
> appropriate here as even when we change the S_NO_SPECIAL (not likely, but
> anyway) value, result of this will be 0 if no match.

I should add an assert.

> Also, I would probably add some comment there to clarify the purpose of the
> assert, like:
> 
> /* This assert prevents misuse of contains_special by checking for more
>  * specials at once. */
> 
> >  }

No problem.


> >  /***************************************************************
> > Index: common/map.h
> > ===================================================================
> > RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
> > retrieving revision 1.118
> > diff -u -r1.118 map.h
> > --- common/map.h    2002/02/14 15:17:14     1.118
> > +++ common/map.h    2002/02/16 16:19:20
> > @@ -263,10 +263,16 @@
> >  void map_set_city(int x, int y, struct city *pcity);
> >  enum tile_terrain_type map_get_terrain(int x, int y);
> >  enum tile_special_type map_get_special(int x, int y);
> > -int map_has_special(int x, int y, enum tile_special_type);
> > +bool map_has_special(int x, int y, enum tile_special_type);
> >  void map_set_terrain(int x, int y, enum tile_terrain_type ter);
> >  void map_set_special(int x, int y, enum tile_special_type spe);
> >  void map_clear_special(int x, int y, enum tile_special_type spe);
> > +
> > +bool tile_has_special(struct tile *ptile,
> > +                 enum tile_special_type to_test_for);
> > +bool contains_special(enum tile_special_type all,
> > +                 enum tile_special_type to_test_for);
> > +
> 
> Why not to group the *_has_special() together?

Ok.

> > Index: server/unittools.c
> > ===================================================================
> > RCS file: /home/freeciv/CVS/freeciv/server/unittools.c,v
> > retrieving revision 1.158
> > diff -u -r1.158 unittools.c
> > --- server/unittools.c      2002/02/14 15:17:41     1.158
> > +++ server/unittools.c      2002/02/16 16:19:34
> > @@ -1547,7 +1547,7 @@
> >  
> >    if ((is_allied_city_tile(map_get_tile(x, y), pplayer)
> >         && !is_non_allied_unit_tile(map_get_tile(x, y), pplayer))
> > -      || (BOOL_VAL(plrtile->special & S_AIRBASE)
> > +      || (contains_special(plrtile->special, S_AIRBASE)
> 
> tile_has_special()
> 
> > @@ -2569,7 +2569,7 @@
> >     map_get_player_tile(x, y, unit_owner(ptrans));
> >      bool is_refuel_point =
> >     is_allied_city_tile(map_get_tile(x, y), unit_owner(ptrans))
> > -   || (BOOL_VAL(plrtile->special & S_AIRBASE)
> > +   || (contains_special(plrtile->special, S_AIRBASE)
> 
> tile_has_special()

It is a player_tile and not a (map_)tile.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  Living on earth may be expensive, but it includes an annual free trip
  around the sun.