<URL: http://bugs.freeciv.org/Ticket/Display.html?id=39827 >

In PR#39562, despite ample warnings in the code, somebody changed
CT_BUILDING from 0 to 1 without checking the choice.type instances.
Sad to say, there were many such instances that assume that CT_NONE
and CT_BUILDING are the same -- more importantly, the only valid
possibility other than is_unit_choice_type().

Since CT_NONE has no valid value (or a NULL value as inconsistently
initialized), the code was passing that missing value for buildings
and causing havoc.

This patch updates each instance of choice.type to differentiate
CT_NONE from CT_BUILDING.  In most places, is_unit_choice_type() is
replaced by a switch, ensuring that all cases are handled (often for
improved error detection).

Most remaining tests for improvement or unit now use a switch, with
opportunities for more error logging.

Revised the PACKET_CITY_CHANGE to use the new universal kind, and
revised some error logging in packhand.c handle_city_info().  (This is
how the bad values were passed between server and client.  Never trust
values from the network!)

More use of previously introduced name_translation functions, instead
of scattered tests for improvement or unit.  Mostly cosmetic, but
hopefully detects/prevents future problems by isolating the tests:
   city_production_name_translation()
   universal_name_translation()

Changed unit.h enum production_class_type TYPE_* to PCT_* -- making
searches for them easier.

Lightly tested, but committing immediately.

Index: version.in
===================================================================
--- version.in  (revision 13908)
+++ version.in  (working copy)
@@ -23,4 +23,4 @@
 #   - Avoid adding a new mandatory capbility to the development branch for
 #     as long as possible.  We want to maintain network compatibility with
 #     the stable branch for as long as possible.
-NETWORK_CAPSTRING="+Freeciv.Devel.2007.Oct.12"
+NETWORK_CAPSTRING="+Freeciv.Devel.2007.Nov.01"
Index: server/cityhand.c
===================================================================
--- server/cityhand.c   (revision 13908)
+++ server/cityhand.c   (working copy)
@@ -324,14 +324,30 @@
 /**************************************************************************
 ...
 **************************************************************************/
-void handle_city_change(struct player *pplayer, int city_id, int build_id,
-                       bool is_build_id_unit_id)
+void handle_city_change(struct player *pplayer, int city_id,
+                       int production_kind, int production_value)
 {
+  struct universal prod;
   struct city *pcity = player_find_city_by_id(pplayer, city_id);
-  struct universal prod =
-    universal_by_number(is_build_id_unit_id ? VUT_UTYPE : VUT_IMPROVEMENT,
-                           build_id);
 
+  if (production_kind < VUT_NONE || production_kind >= VUT_LAST) {
+    freelog(LOG_ERROR, "handle_city_change()"
+            " bad production_kind %d.",
+            production_kind);
+    prod.kind = VUT_NONE;
+    return;
+  } else {
+    prod = universal_by_number(production_kind, production_value);
+    if (prod.kind < VUT_NONE || prod.kind >= VUT_LAST) {
+      freelog(LOG_ERROR, "handle_city_change()"
+              " production_kind %d with bad production_value %d.",
+              production_kind,
+              production_value);
+      prod.kind = VUT_NONE;
+    }
+    return;
+  }
+
   if (!pcity) {
     return;
   }
Index: server/citytools.c
===================================================================
--- server/citytools.c  (revision 13908)
+++ server/citytools.c  (working copy)
@@ -1913,7 +1913,7 @@
     notify_player(NULL, pcity->tile, E_WONDER_STOPPED,
                     _("The %s have stopped building The %s in %s."),
                     nation_plural_for_player(pplayer),
-                    city_improvement_name_translation(pcity, 
pcity->production.value.building),
+                    city_production_name_translation(pcity),
                     pcity->name);
   }
 
@@ -1925,11 +1925,7 @@
   pcity->production = target;
 
   /* What's the name of the target? */
-  if (VUT_UTYPE == target.kind) {
-    name = utype_name_translation(pcity->production.value.utype);
-  } else {
-    name = improvement_name_translation(pcity->production.value.building);
-  }
+  name = city_production_name_translation(pcity);
 
   switch (event) {
     case E_WORKLIST: source = _(" from the worklist"); break;
@@ -1943,19 +1939,19 @@
   /* FIXME: this may give bad grammar when translated if the 'source'
    * string can have multiple values. */
   notify_player(pplayer, pcity->tile, event,
-                  /* TRANS: "<city> is building <production><source>." */
-                  _("%s is building %s%s."),
-                  pcity->name, name, source);
+               /* TRANS: "<city> is building <production><source>." */
+               _("%s is building %s%s."),
+               pcity->name, name, source);
 
   /* If the city is building a wonder, tell the rest of the world
      about it. */
   if (VUT_IMPROVEMENT == pcity->production.kind
    && is_great_wonder(pcity->production.value.building)) {
     notify_player(NULL, pcity->tile, E_WONDER_STARTED,
-                    _("The %s have started building The %s in %s."),
-                    nation_plural_for_player(pplayer),
-                    city_improvement_name_translation(pcity, 
pcity->production.value.building),
-                    pcity->name);
+                 _("The %s have started building The %s in %s."),
+                 nation_plural_for_player(pplayer),
+                 name,
+                 pcity->name);
   }
 }
 
Index: server/cityturn.c
===================================================================
--- server/cityturn.c   (revision 13908)
+++ server/cityturn.c   (working copy)
@@ -156,7 +156,7 @@
 {
   /* The caller had better check this! */
   if (!cmr->found_a_valid) {
-    freelog(LOG_ERROR, "apply_cmresult_to_city() called with non-valid "
+    freelog(LOG_FATAL, "apply_cmresult_to_city() called with invalid "
             "cm_result");
     assert(0);
     return;
@@ -669,6 +669,7 @@
 static bool worklist_change_build_target(struct player *pplayer,
                                         struct city *pcity)
 {
+  struct universal target;
   bool success = FALSE;
   int i;
 
@@ -677,25 +678,21 @@
     return FALSE;
 
   i = 0;
-  while (TRUE) {
-    struct universal target;
+  while (!success && worklist_peek_ith(&pcity->worklist, &target, i++)) {
+    success = can_city_build_now(pcity, target);
+    if (success) {
+      break; /* while */
+    }
 
-    /* What's the next item in the worklist? */
-    if (!worklist_peek_ith(&pcity->worklist, &target, i))
-      /* Nothing more in the worklist.  Ah, well. */
-      break;
-
-    i++;
-
-    /* Sanity checks */
-    if (VUT_UTYPE == target.kind &&
-       !can_city_build_unit_now(pcity, target.value.utype)) {
+    switch (target.kind) {
+    case VUT_UTYPE:
+    {
       struct unit_type *ptarget = target.value.utype;
       struct unit_type *pupdate = unit_upgrades_to(pcity, ptarget);
 
       /* Maybe we can just upgrade the target to what the city /can/ build. */
       if (U_NOT_OBSOLETED == pupdate) {
-       /* Nope, we're stuck.  Dump this item from the worklist. */
+       /* Nope, we're stuck.  Skip this item from the worklist. */
        notify_player(pplayer, pcity->tile, E_CITY_CANTBUILD,
                         _("%s can't build %s from the worklist; "
                           "tech not yet available.  Postponing..."),
@@ -705,8 +702,10 @@
                           API_TYPE_UNIT_TYPE, ptarget,
                           API_TYPE_CITY, pcity,
                           API_TYPE_STRING, "need_tech");
-       continue;
-      } else if (!can_city_build_unit_later(pcity, pupdate)) {
+       break;
+      }
+      success = can_city_build_unit_later(pcity, pupdate);
+      if (!success) {
        /* If the city can never build this unit or its descendants,
         * drop it. */
        notify_player(pplayer, pcity->tile, E_CITY_CANTBUILD,
@@ -722,11 +721,7 @@
                           API_TYPE_CITY, pcity,
                           API_TYPE_STRING, "never");
        /* Purge this worklist item. */
-       worklist_remove(&pcity->worklist, i-1);
-       /* Reset i to index to the now-next element. */
-       i--;
-       
-       continue;
+       worklist_remove(&pcity->worklist, --i);
       } else {
        /* Yep, we can go after pupdate instead.  Joy! */
        notify_player(pplayer, pcity->tile, E_WORKLIST,
@@ -736,13 +731,16 @@
                         pcity->name);
        target.value.utype = pupdate;
       }
-    } else if (VUT_IMPROVEMENT == target.kind
-              && !can_city_build_improvement_now(pcity, 
target.value.building)) {
+      break;
+    }
+    case VUT_IMPROVEMENT:
+    {
       struct impr_type *ptarget = target.value.building;
       struct impr_type *pupdate = building_upgrades_to(pcity, ptarget);
 
       /* If the city can never build this improvement, drop it. */
-      if (!can_city_build_improvement_later(pcity, pupdate)) {
+      success = can_city_build_improvement_later(pcity, pupdate);
+      if (!success) {
        /* Nope, never in a million years. */
        notify_player(pplayer, pcity->tile, E_CITY_CANTBUILD,
                         _("%s can't build %s from the worklist.  "
@@ -755,14 +753,10 @@
                           API_TYPE_STRING, "never");
 
        /* Purge this worklist item. */
-       worklist_remove(&pcity->worklist, i-1);
-       /* Reset i to index to the now-next element. */
-       i--;
-       
-       continue;
+       worklist_remove(&pcity->worklist, --i);
+       break;
       }
 
-
       /* Maybe this improvement has been obsoleted by something that
         we can build. */
       if (pupdate == ptarget) {
@@ -850,14 +844,6 @@
                                 API_TYPE_CITY, pcity,
                                 API_TYPE_STRING, "need_nation");
              break;
-           case VUT_UTYPE:
-           case VUT_UTFLAG:
-           case VUT_UCLASS:
-           case VUT_UCFLAG:
-           case VUT_OTYPE:
-           case VUT_SPECIALIST:
-             /* Will only happen with a bogus ruleset. */
-             break;
            case VUT_MINSIZE:
              notify_player(pplayer, pcity->tile, E_CITY_CANTBUILD,
                               _("%s can't build %s from the worklist; "
@@ -897,12 +883,24 @@
                                 API_TYPE_CITY, pcity,
                                 API_TYPE_STRING, "need_terrainclass");
              break;
+           case VUT_UTYPE:
+           case VUT_UTFLAG:
+           case VUT_UCLASS:
+           case VUT_UCFLAG:
+           case VUT_OTYPE:
+           case VUT_SPECIALIST:
+             /* Will only happen with a bogus ruleset. */
+             freelog(LOG_ERROR, "worklist_change_build_target()"
+                     " has bogus preq");
+             break;
            case VUT_NONE:
            case VUT_LAST:
            default:
+             freelog(LOG_FATAL, "worklist_change_build_target()"
+                     " called with invalid preq");
              assert(0);
              break;
-           }
+           };
            break;
          }
        } requirement_vector_iterate_end;
@@ -915,7 +913,6 @@
                           pcity->name,
                           city_improvement_name_translation(pcity, ptarget));
        }
-       continue;
       } else {
        /* Hey, we can upgrade the improvement!  */
        notify_player(pplayer, pcity->tile, E_WORKLIST,
@@ -924,17 +921,23 @@
                         city_improvement_name_translation(pcity, pupdate),
                         pcity->name);
        target.value.building = pupdate;
+       success = TRUE;
       }
+      break;
     }
+    default:
+      /* skip useless target */
+      freelog(LOG_ERROR, "worklist_change_build_target()"
+             " has unrecognized target kind (%d)",
+             target.kind);
+      break;
+    };
+  } /* while */
 
+  if (success) {
     /* All okay.  Switch targets. */
     change_build_target(pplayer, pcity, target, E_WORKLIST);
 
-    success = TRUE;
-    break;
-  }
-
-  if (success) {
     /* i is the index immediately _after_ the item we're changing to.
        Remove the (i-1)th item from the worklist. */
     worklist_remove(&pcity->worklist, i-1);
@@ -966,15 +969,23 @@
 
   /* Try building the same thing again.  Repeat building doesn't require a
    * call to change_build_target, so just return. */
-  if (VUT_UTYPE == pcity->production.kind) {
+  switch (pcity->production.kind) {
+  case VUT_UTYPE:
     /* We can build a unit again unless it's unique. */
     if (!utype_has_flag(pcity->production.value.utype, F_UNIQUE)) {
       return;
     }
-  } else if (can_city_build_improvement_now(pcity, 
pcity->production.value.building)) {
-    /* We can build space and coinage again, and possibly others. */
-    return;
-  }
+    break;
+  case VUT_IMPROVEMENT:
+    if (can_city_build_improvement_now(pcity, 
pcity->production.value.building)) {
+      /* We can build space and coinage again, and possibly others. */
+      return;
+    }
+    break;
+  default:
+    /* fallthru */
+    break;
+  };
 
   /* Find *something* to do! */
   freelog(LOG_DEBUG, "Trying advisor_choose_build.");
@@ -1355,6 +1366,7 @@
   case VUT_UTYPE:
     return city_build_unit(pplayer, pcity);
   default:
+    /* must never happen! */
     assert(0);
     break;
   };
Index: server/diplomats.c
===================================================================
--- server/diplomats.c  (revision 13908)
+++ server/diplomats.c  (working copy)
@@ -793,7 +793,6 @@
   struct player *cplayer;
   struct impr_type *ptarget;
   int count, which;
-  const char *prod;
   /* Twice as difficult if target is specified. */
   int success_prob = (improvement >= B_LAST ? game.info.diplchance 
                       : game.info.diplchance / 2); 
@@ -938,17 +937,14 @@
 
   /* Now, the fun stuff!  Do the sabotage! */
   if (NULL == ptarget) {
-    /* Sabotage current production. */
+     char prod[256];
 
     /* Do it. */
     pcity->shield_stock = 0;
     nullify_prechange_production(pcity); /* Make it impossible to recover */
 
     /* Report it. */
-    if (VUT_UTYPE == pcity->production.kind)
-      prod = utype_name_translation(pcity->production.value.utype);
-    else
-      prod = improvement_name_translation(pcity->production.value.building);
+    universal_name_translation(&pcity->production, prod, sizeof(prod));
 
     notify_player(pplayer, pcity->tile, E_MY_DIPLOMAT_SABOTAGE,
                     _("Your %s succeeded in destroying"
Index: common/packets.def
===================================================================
--- common/packets.def  (revision 13908)
+++ common/packets.def  (working copy)
@@ -575,8 +575,8 @@
 
 PACKET_CITY_CHANGE=25;cs,dsend
   CITY city_id;
-  UINT8 build_id;
-  BOOL is_build_id_unit_id;
+  UINT8 production_kind;
+  UINT8 production_value;
 end
 
 PACKET_CITY_WORKLIST=26;cs,dsend
@@ -861,7 +861,7 @@
 
 PACKET_UNIT_DIPLOMAT_QUERY=66;cs,handle-per-conn,dsend
   UNIT diplomat_id;
-  UNIT target_id;   # city_id or unit_id target_id;
+  UNIT target_id;   # city_id or unit_id
   SINT16 value;
   DIPLOMAT_ACTION action_type;
 end
@@ -874,14 +874,14 @@
 
 PACKET_UNIT_DIPLOMAT_ACTION=70;cs,dsend
   UNIT diplomat_id;
-  UNIT target_id;   # city_id or unit_id target_id;
+  UNIT target_id;   # city_id or unit_id
   SINT16 value;
   DIPLOMAT_ACTION action_type;
 end
 
 PACKET_UNIT_DIPLOMAT_ANSWER=71;sc,dsend,lsend
   UNIT diplomat_id;
-  UNIT target_id;   # city_id or unit_id target_id;
+  UNIT target_id;   # city_id or unit_id
   GOLD cost;
   DIPLOMAT_ACTION action_type;
 end
Index: common/city.c
===================================================================
--- common/city.c       (revision 13908)
+++ common/city.c       (working copy)
@@ -381,16 +381,16 @@
 **************************************************************************/
 const char *city_production_name_translation(const struct city *pcity)
 {
+  static char buffer[256];
+
   switch (pcity->production.kind) {
   case VUT_IMPROVEMENT:
     return city_improvement_name_translation(pcity, 
pcity->production.value.building);
-  case VUT_UTYPE:
-    return utype_name_translation(pcity->production.value.utype);
   default:
-    /* FIXME: use universal_name_translation() */
+    /* fallthru */
     break;
-  }
-  return _("(unknown)");
+  };
+  return universal_name_translation(&pcity->production, buffer, 
sizeof(buffer));
 }
 
 /**************************************************************************
@@ -427,7 +427,7 @@
                               build);
   default:
     break;
-  }
+  };
   return FC_INFINITY;
 }
 
@@ -580,11 +580,15 @@
 bool can_city_build_direct(const struct city *pcity,
                           struct universal target)
 {
-  if (VUT_UTYPE == target.kind) {
+  switch (target.kind) {
+  case VUT_UTYPE:
     return can_city_build_unit_direct(pcity, target.value.utype);
-  } else {
+  case VUT_IMPROVEMENT:
     return can_city_build_improvement_direct(pcity, target.value.building);
-  }
+  default:
+    break;
+  };
+  return FALSE;
 }
 
 /**************************************************************************
@@ -593,11 +597,15 @@
 bool can_city_build_now(const struct city *pcity,
                        struct universal target)
 {
-  if (VUT_UTYPE == target.kind) {
+  switch (target.kind) {
+  case VUT_UTYPE:
     return can_city_build_unit_now(pcity, target.value.utype);
-  } else {
+  case VUT_IMPROVEMENT:
     return can_city_build_improvement_now(pcity, target.value.building);
-  }
+  default:
+    break;
+  };
+  return FALSE;
 }
 
 /**************************************************************************
@@ -606,11 +614,15 @@
 bool can_city_build_later(const struct city *pcity,
                          struct universal target)
 {
-  if (VUT_UTYPE == target.kind) {
+  switch (target.kind) {
+  case VUT_UTYPE:
     return can_city_build_unit_later(pcity, target.value.utype);
-  } else {
+  case VUT_IMPROVEMENT:
     return can_city_build_improvement_later(pcity, target.value.building);
-  }
+  default:
+    break;
+  };
+  return FALSE;
 }
 
 /****************************************************************************
@@ -1260,7 +1272,7 @@
 
 /**************************************************************************
  Compute and optionally apply the change-production penalty for the given
- production change (to target,is_unit) in the given city (pcity).
+ production change (to target) in the given city (pcity).
  Always returns the number of shields which would be in the stock if
  the penalty had been applied.
 
@@ -1277,24 +1289,41 @@
   enum production_class_type new_class;
   int unpenalized_shields = 0, penalized_shields = 0;
 
-  if (VUT_UTYPE == pcity->changed_from.kind) {
-    orig_class = TYPE_UNIT;
-  } else if (is_wonder(pcity->changed_from.value.building)) {
-    orig_class = TYPE_WONDER;
-  } else {
-    orig_class = TYPE_NORMAL_IMPROVEMENT;
-  }
+  switch (pcity->changed_from.kind) {
+  case VUT_IMPROVEMENT:
+    if (is_wonder(pcity->changed_from.value.building)) {
+      orig_class = PCT_WONDER;
+    } else {
+      orig_class = PCT_NORMAL_IMPROVEMENT;
+    }
+    break;
+  case VUT_UTYPE:
+    orig_class = PCT_UNIT;
+    break;
+  default:
+    orig_class = PCT_LAST;
+    break;
+  };
 
-  if (VUT_UTYPE == target.kind) {
-    new_class = TYPE_UNIT;
-  } else if (is_wonder(target.value.building)) {
-    new_class = TYPE_WONDER;
-  } else {
-    new_class = TYPE_NORMAL_IMPROVEMENT;
-  }
+  switch (target.kind) {
+  case VUT_IMPROVEMENT:
+    if (is_wonder(pcity->changed_from.value.building)) {
+      new_class = PCT_WONDER;
+    } else {
+      new_class = PCT_NORMAL_IMPROVEMENT;
+    }
+    break;
+  case VUT_UTYPE:
+    new_class = PCT_UNIT;
+    break;
+  default:
+    new_class = PCT_LAST;
+    break;
+  };
 
   /* Changing production is penalized under certain circumstances. */
-  if (orig_class == new_class) {
+  if (orig_class == new_class
+   || orig_class == PCT_LAST) {
     /* There's never a penalty for building something of the same class. */
     unpenalized_shields = pcity->before_change_shields;
   } else if (city_built_last_turn(pcity)) {
@@ -1315,7 +1344,7 @@
 
   /* Caravan shields are penalized (just as if you disbanded the caravan)
    * if you're not building a wonder. */
-  if (new_class == TYPE_WONDER) {
+  if (new_class == PCT_WONDER) {
     unpenalized_shields += pcity->caravan_shields;
   } else {
     penalized_shields += pcity->caravan_shields;
Index: common/city.h
===================================================================
--- common/city.h       (revision 13908)
+++ common/city.h       (working copy)
@@ -20,11 +20,16 @@
 #include "worklist.h"
 
 enum production_class_type {
-  TYPE_UNIT, TYPE_NORMAL_IMPROVEMENT, TYPE_WONDER
+  PCT_UNIT,
+  PCT_NORMAL_IMPROVEMENT,
+  PCT_WONDER,
+  PCT_LAST
 };
 
 enum city_tile_type {
-  C_TILE_EMPTY, C_TILE_WORKER, C_TILE_UNAVAILABLE
+  C_TILE_EMPTY,
+  C_TILE_WORKER,
+  C_TILE_UNAVAILABLE
 };
 
 /* Various city options.  These are stored by the server and can be
Index: common/requirements.c
===================================================================
--- common/requirements.c       (revision 13908)
+++ common/requirements.c       (working copy)
@@ -1156,7 +1156,7 @@
 {
   switch (psource->kind) {
   case VUT_NONE:
-    return "(none)";
+    return N_("(none)");
   case VUT_ADVANCE:
     return advance_rule_name(psource->value.advance);
   case VUT_GOVERNMENT:
@@ -1182,7 +1182,7 @@
   case VUT_SPECIALIST:
     return specialist_rule_name(psource->value.specialist);
   case VUT_MINSIZE:
-    return "Size %d";
+    return N_("Size %d");
   case VUT_AI_LEVEL:
     return ai_level_name(psource->value.ai_level);
   case VUT_TERRAINCLASS:
@@ -1258,10 +1258,11 @@
     cat_snprintf(buf, bufsz, _("%s AI"),
                  ai_level_name(psource->value.ai_level)); /* FIXME */
     break;
-   case VUT_TERRAINCLASS:
-     cat_snprintf(buf, bufsz, _("%s terrain"),
-                  terrain_class_name_translation(psource->value.terrainclass));
-     break;
+  case VUT_TERRAINCLASS:
+    /* TRANS: "Land terrain" */
+    cat_snprintf(buf, bufsz, _("%s terrain"),
+                 terrain_class_name_translation(psource->value.terrainclass));
+    break;
   case VUT_LAST:
     assert(0);
     break;
Index: ai/aitools.c
===================================================================
--- ai/aitools.c        (revision 13908)
+++ ai/aitools.c        (working copy)
@@ -1156,21 +1156,6 @@
 }
 
 /**************************************************************************
-  ...
-**************************************************************************/
-void city_production_from_ai_choice(struct universal *product,
-                                   struct ai_choice *choice)
-{
-  if (is_unit_choice_type(choice->type)) {
-    product->kind = VUT_UTYPE;
-    product->value.utype = choice->value.utype;
-  } else {
-    product->kind = VUT_IMPROVEMENT;
-    product->value.building = choice->value.building;
-  }
-}
-
-/**************************************************************************
   Calls ai_wants_role_unit to choose the best unit with the given role and 
   set tech wants.  Sets choice->value.utype when we can build something.
 **************************************************************************/
Index: ai/aitools.h
===================================================================
--- ai/aitools.h        (revision 13908)
+++ ai/aitools.h        (working copy)
@@ -94,8 +94,6 @@
 void copy_if_better_choice(struct ai_choice *cur, struct ai_choice *best);
 
 bool is_unit_choice_type(enum choice_type type);
-void city_production_from_ai_choice(struct universal *product,
-                                   struct ai_choice *choice);
 
 bool ai_choose_role_unit(struct player *pplayer, struct city *pcity,
                          struct ai_choice *choice, enum choice_type type,
Index: ai/aicity.c
===================================================================
--- ai/aicity.c (revision 13908)
+++ ai/aicity.c (working copy)
@@ -89,8 +89,8 @@
   || pcity->food_stock + pcity->surplus[O_FOOD] < 0)
 #define LOG_BUY LOG_DEBUG
 
+static void ai_sell_obsolete_buildings(struct city *pcity);
 static void resolve_city_emergency(struct player *pplayer, struct city *pcity);
-static void ai_sell_obsolete_buildings(struct city *pcity);
 
 /**************************************************************************
   Return the number of "luxury specialists".  This is the number of
@@ -1382,41 +1382,67 @@
     }
   }
 
-  if (pcity->ai.choice.want != 0) { 
+  if (pcity->ai.choice.want != 0) {
+    const char *name = "(unknown)";
     ASSERT_CHOICE(pcity->ai.choice);
 
+    switch (pcity->ai.choice.type) {
+    case CT_CIVILIAN:
+    case CT_ATTACKER:
+    case CT_DEFENDER:
+      name = utype_rule_name(pcity->ai.choice.value.utype);
+      break;
+    case CT_BUILDING:
+      name = improvement_rule_name(pcity->ai.choice.value.building);
+      break;
+    case CT_NONE:
+    case CT_LAST:
+      break;
+    };
     CITY_LOG(LOG_DEBUG, pcity, "wants %s with desire %d.",
-            is_unit_choice_type(pcity->ai.choice.type)
-            ? utype_rule_name(pcity->ai.choice.value.utype)
-            : improvement_rule_name(pcity->ai.choice.value.building),
+            name,
             pcity->ai.choice.want);
     
+    /* parallel to citytools change_build_target() */
     if (VUT_IMPROVEMENT == pcity->production.kind
      && is_great_wonder(pcity->production.value.building)
-     && (is_unit_choice_type(pcity->ai.choice.type)
+     && (CT_BUILDING != pcity->ai.choice.type
       || pcity->ai.choice.value.building != pcity->production.value.building)) 
{
       notify_player(NULL, pcity->tile, E_WONDER_STOPPED,
-                      _("The %s have stopped building The %s in %s."),
-                      nation_plural_for_player(pplayer),
-                      city_improvement_name_translation(pcity, 
pcity->production.value.building),
-                      pcity->name);
+                   _("The %s have stopped building The %s in %s."),
+                   nation_plural_for_player(pplayer),
+                   city_production_name_translation(pcity),
+                   pcity->name);
     }
-    if (pcity->ai.choice.type == CT_BUILDING 
-       && is_wonder(pcity->ai.choice.value.building)
-       && (VUT_UTYPE == pcity->production.kind 
-           || pcity->production.value.building != 
pcity->ai.choice.value.building)) {
-      if (is_great_wonder(pcity->ai.choice.value.building)) {
-       notify_player(NULL, pcity->tile, E_WONDER_STARTED,
-                        _("The %s have started building The %s in %s."),
-                        nation_plural_translation(nation_of_city(pcity)),
-                        city_improvement_name_translation(pcity, 
pcity->ai.choice.value.building),
-                        pcity->name);
-      }
-      city_production_from_ai_choice(&pcity->production, &pcity->ai.choice);
-    } else {
-      /* FIXME: same code twice (zero want values?) */
-      city_production_from_ai_choice(&pcity->production, &pcity->ai.choice);
+    if (CT_BUILDING == pcity->ai.choice.type
+      && is_great_wonder(pcity->ai.choice.value.building)
+      && (VUT_IMPROVEMENT != pcity->production.kind
+       || pcity->production.value.building != 
pcity->ai.choice.value.building)) {
+      notify_player(NULL, pcity->tile, E_WONDER_STARTED,
+                   _("The %s have started building The %s in %s."),
+                   nation_plural_translation(nation_of_city(pcity)),
+                   city_improvement_name_translation(pcity, 
pcity->ai.choice.value.building),
+                   pcity->name);
     }
+
+    switch (pcity->ai.choice.type) {
+    case CT_CIVILIAN:
+    case CT_ATTACKER:
+    case CT_DEFENDER:
+      pcity->production.kind = VUT_UTYPE;
+      pcity->production.value.utype = pcity->ai.choice.value.utype;
+      break;
+    case CT_BUILDING:
+      pcity->production.kind = VUT_IMPROVEMENT;
+      pcity->production.value.building = pcity->ai.choice.value.building;
+      break;
+    case CT_NONE:
+      pcity->production.kind = VUT_NONE;
+      break;
+    case CT_LAST:
+      pcity->production.kind = VUT_LAST;
+      break;
+    };
   }
 }
 
@@ -1511,10 +1537,11 @@
   } city_list_iterate_end;
   
   do {
+    bool expensive; /* don't buy when it costs x2 unless we must */
+    int buycost;
     int limit = cached_limit; /* cached_limit is our gold reserve */
     struct city *pcity = NULL;
-    bool expensive; /* don't buy when it costs x2 unless we must */
-    int buycost;
+    const char *name = "(unknown)";
 
     /* Find highest wanted item on the buy list */
     init_choice(&bestchoice);
@@ -1565,7 +1592,7 @@
       continue; /* Already completed */
     }
 
-    if (bestchoice.type != CT_BUILDING
+    if (is_unit_choice_type(bestchoice.type)
         && utype_has_flag(bestchoice.value.utype, F_CITIES)) {
       if (get_city_bonus(pcity, EFT_GROWTH_FOOD) == 0
           && pcity->size == 1
@@ -1600,6 +1627,20 @@
        continue;
     }
 
+    switch (bestchoice.type) {
+    case CT_CIVILIAN:
+    case CT_ATTACKER:
+    case CT_DEFENDER:
+      name = utype_rule_name(bestchoice.value.utype);
+      break;
+    case CT_BUILDING:
+      name = improvement_rule_name(bestchoice.value.building);
+      break;
+    case CT_NONE:
+    case CT_LAST:
+      break;
+    };
+
     /* FIXME: Here Syela wanted some code to check if
      * pcity was doomed, and we should therefore attempt
      * to sell everything in it of non-military value */
@@ -1610,9 +1651,7 @@
             || (bestchoice.want > 200 && pcity->ai.urgency > 1))) {
       /* Buy stuff */
       CITY_LOG(LOG_BUY, pcity, "Crash buy of %s for %d (want %d)",
-               bestchoice.type != CT_BUILDING
-              ? utype_rule_name(bestchoice.value.utype)
-               : improvement_rule_name(bestchoice.value.building),
+               name,
                buycost,
                bestchoice.want);
       really_handle_city_buy(pplayer, pcity);
@@ -1621,7 +1660,7 @@
                && assess_defense(pcity) == 0) {
       /* We have no gold but MUST have a defender */
       CITY_LOG(LOG_BUY, pcity, "must have %s but can't afford it (%d < %d)!",
-              utype_rule_name(bestchoice.value.utype),
+              name,
               pplayer->economic.gold, buycost);
       try_to_sell_stuff(pplayer, pcity);
       if (pplayer->economic.gold - pplayer->ai.est_upkeep >= buycost) {
Index: ai/advmilitary.c
===================================================================
--- ai/advmilitary.c    (revision 13908)
+++ ai/advmilitary.c    (working copy)
@@ -1491,13 +1491,24 @@
 
   if (choice->want <= 0) {
     CITY_LOG(LOGLEVEL_BUILD, pcity, "military advisor has no advice");
-  } else if (is_unit_choice_type(choice->type)) {
-    CITY_LOG(LOGLEVEL_BUILD, pcity, "military advisor choice: %s (want %d)",
-             utype_rule_name(choice->value.utype),
-             choice->want);
   } else {
+    const char *name = "(unknown)";
+
+    switch (pcity->ai.choice.type) {
+    case CT_CIVILIAN:
+    case CT_ATTACKER:
+    case CT_DEFENDER:
+      name = utype_rule_name(choice->value.utype);
+      break;
+    case CT_BUILDING:
+      name = improvement_rule_name(choice->value.building);
+      break;
+    case CT_NONE:
+    case CT_LAST:
+      break;
+    };
     CITY_LOG(LOGLEVEL_BUILD, pcity, "military advisor choice: %s (want %d)",
-             improvement_rule_name(choice->value.building),
+             name,
              choice->want);
   }
 }
Index: client/citydlg_common.c
===================================================================
--- client/citydlg_common.c     (revision 13908)
+++ client/citydlg_common.c     (working copy)
@@ -259,29 +259,33 @@
                                     struct universal target,
                                     struct city *pcity)
 {
-  if (VUT_IMPROVEMENT == target.kind
-   && improvement_has_flag(target.value.building, IF_GOLD)) {
-    my_snprintf(buffer, buffer_len, _("%s (XX) %d/turn"),
-               city_improvement_name_translation(pcity, target.value.building),
-               MAX(0, pcity->surplus[O_SHIELD]));
-  } else {
-    const char *name;
-    int turns = city_turns_to_build(pcity, target, TRUE);
-    int cost= universal_build_shield_cost(&target);
+  int turns = city_turns_to_build(pcity, target, TRUE);
+  int cost = universal_build_shield_cost(&target);
 
-    if (VUT_UTYPE == target.kind) {
-      name = utype_values_translation(target.value.utype);
-    } else {
-      name = city_improvement_name_translation(pcity, target.value.building);
+  switch (target.kind) {
+  case VUT_IMPROVEMENT:
+    my_snprintf(buffer, buffer_len,
+                city_improvement_name_translation(pcity, 
target.value.building));
+
+    if (improvement_has_flag(target.value.building, IF_GOLD)) {
+      cat_snprintf(buffer, buffer_len, " (--) ");
+      cat_snprintf(buffer, buffer_len, _("%d/turn"),
+                  MAX(0, pcity->surplus[O_SHIELD]));
+      return;
     }
+    break;
+  default:
+    universal_name_translation(&target, buffer, buffer_len);
+    break;
+  };
+  cat_snprintf(buffer, buffer_len, " (%d) ", cost);
 
-    if (turns < FC_INFINITY) {
-      my_snprintf(buffer, buffer_len,
-                 PL_("%s (%d) %d turn", "%s (%d) %d turns", turns),
-                 name, cost, turns);
-    } else {
-      my_snprintf(buffer, buffer_len, "%s (%d) never", name, cost);
-    }
+  if (turns < FC_INFINITY) {
+    cat_snprintf(buffer, buffer_len,
+                PL_("%d turn", "%d turns", turns),
+                turns);
+  } else {
+    cat_snprintf(buffer, buffer_len, "never");
   }
 }
 
@@ -294,24 +298,27 @@
                                    struct universal target,
                                    struct city *pcity)
 {
-  if (VUT_UTYPE == target.kind) {
+  universal_name_translation(&target, buf[0], column_size);
+
+  switch (target.kind) {
+  case VUT_UTYPE:
+  {
     struct unit_type *ptype = target.value.utype;
 
-    my_snprintf(buf[0], column_size, utype_name_translation(ptype));
     my_snprintf(buf[1], column_size, utype_values_string(ptype));
     my_snprintf(buf[2], column_size, "(%d)", utype_build_shield_cost(ptype));
-  } else {
+    break;
+  }
+  case VUT_IMPROVEMENT:
+  {
     struct player *pplayer = pcity ? pcity->owner : game.player_ptr;
     struct impr_type *pimprove = target.value.building;
 
     /* Total & turns left meaningless on capitalization */
-    if (improvement_has_flag(target.value.building, IF_GOLD)) {
-      my_snprintf(buf[0], column_size, improvement_name_translation(pimprove));
+    if (improvement_has_flag(pimprove, IF_GOLD)) {
       buf[1][0] = '\0';
       my_snprintf(buf[2], column_size, "---");
     } else {
-      my_snprintf(buf[0], column_size, improvement_name_translation(pimprove));
-
       /* from city.c city_improvement_name_translation() */
       if (pcity && is_building_replaced(pcity, pimprove, RPT_CERTAIN)) {
        my_snprintf(buf[1], column_size, "*");
@@ -335,13 +342,19 @@
             state = _("Small Wonder");
           }
        }
-       my_snprintf(buf[1], column_size, "%s", state);
+       my_snprintf(buf[1], column_size, state);
       }
 
       my_snprintf(buf[2], column_size, "%d",
                  impr_build_shield_cost(pimprove));
     }
+    break;
   }
+  default:
+    buf[1][0] = '\0';
+    buf[2][0] = '\0';
+    break;
+  };
 
   /* Add the turns-to-build entry in the 4th position */
   if (pcity) {
@@ -355,7 +368,7 @@
       if (turns < FC_INFINITY) {
        my_snprintf(buf[3], column_size, "%d", turns);
       } else {
-       my_snprintf(buf[3], column_size, "%s", _("never"));
+       my_snprintf(buf[3], column_size, _("never"));
       }
     }
   } else {
@@ -587,8 +600,8 @@
 int city_change_production(struct city *pcity, struct universal target)
 {
   return dsend_packet_city_change(&aconnection, pcity->id,
-                                 universal_number(&target),
-                                 VUT_UTYPE == target.kind);
+                                 target.kind,
+                                 universal_number(&target));
 }
 
 /**************************************************************************
Index: client/gui-gtk-2.0/cityrep.c
===================================================================
--- client/gui-gtk-2.0/cityrep.c        (revision 13908)
+++ client/gui-gtk-2.0/cityrep.c        (working copy)
@@ -976,11 +976,11 @@
     itree_get(&it, 0, &res, -1);
     pcity = res;
 
-    if ( (which == TYPE_UNIT && VUT_UTYPE == pcity->production.kind)
-         || (which == TYPE_NORMAL_IMPROVEMENT
+    if ( (which == PCT_UNIT && VUT_UTYPE == pcity->production.kind)
+         || (which == PCT_NORMAL_IMPROVEMENT
              && VUT_IMPROVEMENT == pcity->production.kind
              && !is_wonder(pcity->production.value.building))
-         || (which == TYPE_WONDER
+         || (which == PCT_WONDER
              && VUT_IMPROVEMENT == pcity->production.kind
              && is_wonder(pcity->production.value.building)) ) {
       itree_select(city_selection, &it);
@@ -1406,19 +1406,19 @@
   gtk_menu_shell_append(GTK_MENU_SHELL(menu), item);
   g_signal_connect(item, "activate",
                   G_CALLBACK(city_select_building_callback),
-                  GINT_TO_POINTER(TYPE_UNIT));
+                  GINT_TO_POINTER(PCT_UNIT));
 
   item = gtk_menu_item_new_with_label( _("Building Improvements"));
   gtk_menu_shell_append(GTK_MENU_SHELL(menu), item);
   g_signal_connect(item, "activate",
                   G_CALLBACK(city_select_building_callback),
-                  GINT_TO_POINTER(TYPE_NORMAL_IMPROVEMENT));
+                  GINT_TO_POINTER(PCT_NORMAL_IMPROVEMENT));
 
   item = gtk_menu_item_new_with_label(_("Building Wonders"));
   gtk_menu_shell_append(GTK_MENU_SHELL(menu), item);
   g_signal_connect(item, "activate",
                   G_CALLBACK(city_select_building_callback),
-                  GINT_TO_POINTER(TYPE_WONDER));
+                  GINT_TO_POINTER(PCT_WONDER));
 
 
   item = gtk_separator_menu_item_new();
Index: client/packhand.c
===================================================================
--- client/packhand.c   (revision 13908)
+++ client/packhand.c   (working copy)
@@ -426,14 +426,17 @@
   }
 
   if (packet->production_kind < VUT_NONE || packet->production_kind >= 
VUT_LAST) {
-    freelog(LOG_ERROR, "handle_city_info() bad production_kind %d.",
+    freelog(LOG_ERROR, "handle_city_info()"
+            " bad production_kind %d.",
             packet->production_kind);
     product.kind = VUT_NONE;
   } else {
     product = universal_by_number(packet->production_kind,
-                                     packet->production_value);
-    if (product.kind < VUT_NONE ||  product.kind >= VUT_LAST) {
-      freelog(LOG_ERROR, "handle_city_info() bad production_value %d.",
+                                  packet->production_value);
+    if (product.kind < VUT_NONE || product.kind >= VUT_LAST) {
+      freelog(LOG_ERROR, "handle_city_info()"
+              " production_kind %d with bad production_value %d.",
+              packet->production_kind,
               packet->production_value);
       product.kind = VUT_NONE;
     }
Index: client/mapctrl_common.c
===================================================================
--- client/mapctrl_common.c     (revision 13908)
+++ client/mapctrl_common.c     (working copy)
@@ -364,6 +364,7 @@
 **************************************************************************/
 void clipboard_copy_production(struct tile *ptile)
 {
+  char buffer[256];
   struct city *pcity = ptile->city;
 
   if (!can_client_issue_orders()) {
@@ -393,9 +394,7 @@
 
   create_event(ptile, E_CITY_PRODUCTION_CHANGED, /* ? */
               _("Copy %s to clipboard."),
-              VUT_UTYPE == clipboard.kind
-              ? utype_name_translation(clipboard.value.utype)
-              : improvement_name_translation(clipboard.value.building));
+              universal_name_translation(&clipboard, buffer, sizeof(buffer)));
 }
 
 /**************************************************************************
@@ -439,8 +438,8 @@
   }
 
   dsend_packet_city_change(&aconnection, pcity->id,
-                          universal_number(&clipboard),
-                          VUT_UTYPE == clipboard.kind);
+                          clipboard.kind,
+                          universal_number(&clipboard));
 }
 
 /**************************************************************************
Index: client/mapview_common.c
===================================================================
--- client/mapview_common.c     (revision 13908)
+++ client/mapview_common.c     (working copy)
@@ -1912,35 +1912,23 @@
 void get_city_mapview_production(struct city *pcity,
                                  char *buffer, size_t buffer_len)
 {
-  int turns = city_production_turns_to_build(pcity, TRUE);
-  /* FIXME: rewrite with universal_name_translation and concatenation */
-                               
-  if (VUT_UTYPE == pcity->production.kind) {
-    struct unit_type *punit_type = pcity->production.value.utype;
-    if (turns < 999) {
-      my_snprintf(buffer, buffer_len, "%s %d",
-                  utype_name_translation(punit_type),
-                  turns);
-    } else {
-      my_snprintf(buffer, buffer_len, "%s -",
-                  utype_name_translation(punit_type));
-    }
+  int turns;
+
+  universal_name_translation(&pcity->production, buffer, buffer_len);
+
+  if (city_production_has_flag(pcity, IF_GOLD)) {
+    return;
+  }
+  turns = city_production_turns_to_build(pcity, TRUE);
+
+  if (999 < turns) {
+    cat_snprintf(buffer, buffer_len, " -");
   } else {
-    struct impr_type *pimprove = pcity->production.value.building;
-    if (improvement_has_flag(pimprove, IF_GOLD)) {
-      my_snprintf(buffer, buffer_len, "%s",
-                 improvement_name_translation(pimprove));
-    } else if (turns < 999) {
-      my_snprintf(buffer, buffer_len, "%s %d",
-                 improvement_name_translation(pimprove),
-                 turns);
-    } else {
-      my_snprintf(buffer, buffer_len, "%s -",
-                 improvement_name_translation(pimprove));
-    }
+    cat_snprintf(buffer, buffer_len, " %d", turns);
   }
 }
 
+/***************************************************************************/
 static enum update_type needed_updates = UPDATE_NONE;
 static bool callback_queued = FALSE;
 
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to