[Freeciv-Dev] Re: [Patch] Boolean cleanup in ai/

[Petr Baudis <pasky@xxxxxxxxxxx>]

No general objections to the patch (congrats!); it's mostly ready for commit,
didn't notice anything really weird. There's one minor global objection - more
usage of > 0 instead of != 0, but I think this should be done rather in local
cleanups and the current change keeps the behaviour identical.

I really tried to read the whole patch, even when it took about half an hour,
but I wouldn't like to do this once again. So I highly suggest to divide the
commit to more stages, committing everything noone objects against now (after
someone else will have a look at this as well), and include only controverse
parts in a next patch.

My only objection and piece of code I don't want inside is the gothere() in
findvictim(). If it compiles and autogames are identical, I think it can go in.

> Index: ai/advmilitary.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/ai/advmilitary.c,v
> retrieving revision 1.86
> diff -u -r1.86 advmilitary.c
> --- ai/advmilitary.c  2002/02/11 10:37:35     1.86
> +++ ai/advmilitary.c  2002/02/18 20:25:55
> @@ -627,11 +628,11 @@
>        }
>        if (b0 > 0) {
>          b0 -= l * SHIELD_WEIGHTING;
> -        b0 -= c * (unhap ? SHIELD_WEIGHTING + 2 * TRADE_WEIGHTING : 
> SHIELD_WEIGHTING);
> +        b0 -= c * (unhap != 0 ? SHIELD_WEIGHTING + 2 * TRADE_WEIGHTING : 
> SHIELD_WEIGHTING);
>        }
>        e = military_amortize(b0, MAX(1, c), fprime + needferry);
>        if (e > 0) {
> -        if (k) {
> +        if (k != 0) {

k > 0 ? let's be consistent

>            pplayer->ai.tech_want[j] += e;
>         if (movetype == SEA_MOVING) {    /* huh? */
>           freelog(LOG_DEBUG,
> @@ -1080,22 +1082,22 @@
>  void establish_city_distances(struct player *pplayer, struct city *pcity)
>  {
>    int dist;
> -  int wondercity;
> +  int wonder_continent;
>    Unit_Type_id freight;
>    int moverate;
>  /* at this moment, our warmap is intact.  we need to do two things: */
>  /* establish faraway for THIS city, and establish d_t_w_c for ALL cities */
>  
>    if (!pcity->is_building_unit && is_wonder(pcity->currently_building))
> -    wondercity = map_get_continent(pcity->x, pcity->y);
> -  else wondercity = 0;
> +    wonder_continent = map_get_continent(pcity->x, pcity->y);
> +  else wonder_continent = 0;
>    freight = best_role_unit(pcity, F_CARAVAN);
>    moverate = (freight==U_LAST) ? SINGLE_MOVE : 
> get_unit_type(freight)->move_rate;
>  
>    pcity->ai.downtown = 0;
>    city_list_iterate(pplayer->cities, othercity)
>      dist = warmap.cost[othercity->x][othercity->y];
> -    if (wondercity && map_get_continent(othercity->x, othercity->y) == 
> wondercity)
> +    if (wonder_continent != 0 && map_get_continent(othercity->x, 
> othercity->y) == wonder_continent)
>        othercity->ai.distance_to_wonder_city = dist;
>      dist += moverate - 1; dist /= moverate;
>      pcity->ai.downtown += MAX(0, 5 - dist); /* four three two one fire */

Well, this doesn't belong in this patch entirely, but I'm completely ok with it
:).

> Index: ai/aiunit.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/ai/aiunit.c,v
> retrieving revision 1.171
> diff -u -r1.171 aiunit.c
> --- ai/aiunit.c       2002/02/13 05:55:20     1.171
> +++ ai/aiunit.c       2002/02/18 20:26:00
> @@ -816,17 +816,17 @@
>          }
>        }
>        if (map_has_special(x1, y1, S_HUT) && best < 99999 &&
> -          could_unit_move_to_tile(punit, punit->x, punit->y, x1, y1) &&
> +          could_unit_move_to_tile(punit, punit->x, punit->y, x1, y1) != 0 &&

Huh. The tristate it returns has no use anyway; so can't we state this function
as ZOC-independent and return boolean? Like we did with ai_military_gothere().

>            !is_barbarian(unit_owner(punit)) &&
>  /*          zoc_ok_move(punit, x1, y1) && !is_sailing_unit(punit) &&*/
>            punit->ai.ai_role != AIUNIT_ESCORT && /* makes life easier */
> -          !punit->ai.charge && /* above line seems not to work. :( */
> +          punit->ai.charge == 0 && /* above line seems not to work. :( */
>            punit->ai.ai_role != AIUNIT_DEFEND_HOME) { /* Oops! -- Syela */
>          best = 99998; *dest_y = y1; *dest_x = x1;
>        }
>        if( is_land_barbarian(pplayer) && best == 0 &&
> -          get_tile_infrastructure_set(map_get_tile(x1, y1)) &&
> -          could_unit_move_to_tile(punit, punit->x, punit->y, x1, y1) ) {
> +          get_tile_infrastructure_set(map_get_tile(x1, y1)) != 0 &&
> +          could_unit_move_to_tile(punit, punit->x, punit->y, x1, y1) != 0) {
>          best = 1; *dest_y = y1; *dest_x = x1;
>        } /* next to nothing is better than nothing */
>      }

-- 

                                Petr "Pasky" Baudis

* 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/