[Freeciv-Dev] Re: [Patch] Special testing
[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
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 ;-).
> 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? :).
> Index: client/gui-win32/mapview.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/client/gui-win32/mapview.c,v
> retrieving revision 1.20
> diff -u -r1.20 mapview.c
> --- client/gui-win32/mapview.c 2002/02/12 08:42:24 1.20
> +++ client/gui-win32/mapview.c 2002/02/16 16:19:14
> @@ -1878,19 +1878,19 @@
> 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(hdc,
> 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(hdc,
> 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(hdc,
> canvas_x, canvas_y,
> sprites.tx.pollution,
> @@ -1927,7 +1927,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(hdc,
> canvas_x, canvas_y-NORMAL_TILE_HEIGHT/2,
> sprites.tx.fortress,
Ditto.
> 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.
> 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? 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).
> @@ -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.
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. */
> }
>
> /***************************************************************
> 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?
> void tile_init(struct tile *ptile);
> bool is_real_tile(int x, int y);
> bool is_normal_map_pos(int x, int y);
> Index: server/sanitycheck.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/server/sanitycheck.c,v
> retrieving revision 1.18
> diff -u -r1.18 sanitycheck.c
> --- server/sanitycheck.c 2002/02/12 08:42:27 1.18
> +++ server/sanitycheck.c 2002/02/16 16:19:27
> @@ -35,16 +35,16 @@
> int terrain = map_get_terrain(x, y);
> int special = map_get_special(x, y);
>
> - if (BOOL_VAL(special & S_RAILROAD))
> + if (contains_special(special, S_RAILROAD))
> assert(special & S_ROAD);
> - if (BOOL_VAL(special & S_FARMLAND))
> + if (contains_special(special, S_FARMLAND))
> assert(special & S_IRRIGATION);
> - if (BOOL_VAL(special & S_SPECIAL_1))
> + if (contains_special(special, S_SPECIAL_1))
> assert(!(special & S_SPECIAL_2));
>
> - if (BOOL_VAL(special & S_MINE))
> + if (contains_special(special, S_MINE))
> assert(get_tile_type(terrain)->mining_result == terrain);
> - if (BOOL_VAL(special & S_IRRIGATION))
> + if (contains_special(special, S_IRRIGATION))
> assert(get_tile_type(terrain)->irrigation_result == terrain);
>
> assert(terrain >= T_ARCTIC && terrain < T_UNKNOWN);
map_has_special()
> Index: server/settlers.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/server/settlers.c,v
> retrieving revision 1.124
> diff -u -r1.124 settlers.c
> --- server/settlers.c 2002/02/14 15:17:38 1.124
> +++ server/settlers.c 2002/02/16 16:19:30
> @@ -478,7 +478,7 @@
> t=map_get_terrain(x,y);
> if (t == T_OCEAN || t == T_RIVER) return TRUE;
> s=map_get_special(x,y);
> - if (BOOL_VAL(s & S_RIVER) || BOOL_VAL(s & S_IRRIGATION)) return TRUE;
> + if (contains_special(s, S_RIVER) || contains_special(s, S_IRRIGATION))
> return TRUE;
map_has_special()
> 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()
--
Petr "Pasky" Baudis,
the encapsulation maniac ;-))
* elinks maintainer * IPv6 guy (XS26 co-coordinator)
* IRCnet operator * FreeCiv AI hacker
.
No one can feel as helpless as the owner of a sick goldfish.
.
Public PGP key && geekcode && homepage: http://pasky.ji.cz/~pasky/
|
|
