<URL: http://bugs.freeciv.org/Ticket/Display.html?id=39835 >
Another problem found with PR#39562, this time on its stated purpose,
better choice asserts. The assert() itself tests both ">= CT_NONE" and
"!= CT_NONE" -- the previous version had commented out "!= CT_NONE".
The assert() also doesn't use standardized *_count() access functions.
Why the "do {} while(0)"? Oh well, I left that, it doesn't seem to do
any harm....
===
PR#39565 removed some error detection done by PR#39553 -- for
building/improvements -- in a section labeled (long before PR#39553):
/* This should never happen? */
Obviously, it was happening! But the logging didn't work, because the
old code (also before PR#39553) used the value directly without
checking for a valid improvement before calling the function:
- } else if (can_build_improvement(pcity,
- punittype->impr_requirement)) {
+ } else if (can_city_build_improvement_now(pcity, impr_req)) {
Instead of finding and fixing the bug, remove the log?!?! That's
counterproductive. It was a simple (NULL != impr_req) fix. Exactly
the kind of bug that copious error detection was intended to find!
There was more effort to stick 4 useless lines around the log and
generate a PR.... And even more effort by me to find and undo the
useless lines several months later. Maybe now we can discover the
underlying problem(s).
===
So that this report isn't a completely painful waste of several hours
time, added ai_choice_rule_name() to replace 3 switches introduced in
PR#39827. Wish I'd thought of it before....
Changed the name of ai_unit_task_rule_name() to conform to practice.
Check simple_ai_types[] for "A_NEVER != punittype->require_advance"
in its update function, rather than at every use of its iterator.
(Some didn't check.)
Index: common/city.h
===================================================================
--- common/city.h (revision 13913)
+++ common/city.h (working copy)
@@ -166,13 +166,13 @@
#define ASSERT_CHOICE(c) \
do { \
if ((c).want > 0) { \
- assert((c).type >= 0 && (c).type < CT_LAST && (c).type != CT_NONE);\
+ assert((c).type > CT_NONE && (c).type < CT_LAST); \
if ((c).type == CT_BUILDING) { \
int _iindex = improvement_index((c).value.building); \
- assert(_iindex >= 0 && _iindex < game.control.num_impr_types); \
+ assert(_iindex >= 0 && _iindex < improvement_count()); \
} else { \
int _uindex = utype_index((c).value.utype); \
- assert(_uindex >= 0 && _uindex < game.control.num_unit_types); \
+ assert(_uindex >= 0 && _uindex < utype_count()); \
} \
} \
} while(0);
Index: ai/aitools.c
===================================================================
--- ai/aitools.c (revision 13913)
+++ ai/aitools.c (working copy)
@@ -59,9 +59,10 @@
#include "aitools.h"
/**************************************************************************
- Return a string describing a unit's AI role.
+ Return the (untranslated) rule name of the ai_unit_task.
+ You don't have to free the return pointer.
**************************************************************************/
-const char *get_ai_role_str(enum ai_unit_task task)
+const char *ai_unit_task_rule_name(const enum ai_unit_task task)
{
switch(task) {
case AIUNIT_NONE:
@@ -83,11 +84,35 @@
case AIUNIT_HUNTER:
return "Hunter";
}
+ /* no default, ensure all types handled somehow */
assert(FALSE);
return NULL;
}
/**************************************************************************
+ Return the (untranslated) rule name of the ai_choice.
+ You don't have to free the return pointer.
+**************************************************************************/
+const char *ai_choice_rule_name(const struct ai_choice *choice)
+{
+ switch (choice->type) {
+ case CT_NONE:
+ return "(nothing)";
+ case CT_BUILDING:
+ return improvement_rule_name(choice->value.building);
+ case CT_CIVILIAN:
+ case CT_ATTACKER:
+ case CT_DEFENDER:
+ return utype_rule_name(choice->value.utype);
+ case CT_LAST:
+ return "(unknown)";
+ };
+ /* no default, ensure all types handled somehow */
+ assert(FALSE);
+ return NULL;
+}
+
+/**************************************************************************
Amortize a want modified by the shields (build_cost) we risk losing.
We add the build time of the unit(s) we risk to amortize delay. The
build time is claculated as the build cost divided by the production
@@ -806,7 +831,8 @@
assert(!unit_has_orders(punit));
UNIT_LOG(LOG_DEBUG, punit, "changing role from %s to %s",
- get_ai_role_str(punit->ai.ai_role), get_ai_role_str(task));
+ ai_unit_task_rule_name(punit->ai.ai_role),
+ ai_unit_task_rule_name(task));
/* Free our ferry. Most likely it has been done already. */
if (task == AIUNIT_NONE || task == AIUNIT_DEFEND_HOME) {
Index: ai/aitools.h
===================================================================
--- ai/aitools.h (revision 13913)
+++ ai/aitools.h (working copy)
@@ -51,7 +51,8 @@
double enemy_zoc_cost;
};
-const char *get_ai_role_str(enum ai_unit_task task);
+const char *ai_unit_task_rule_name(const enum ai_unit_task task);
+const char *ai_choice_rule_name(const struct ai_choice *choice);
int military_amortize(struct player *pplayer, struct city *pcity,
int value, int delay, int build_cost);
Index: ai/aicity.c
===================================================================
--- ai/aicity.c (revision 13913)
+++ ai/aicity.c (working copy)
@@ -1383,24 +1383,10 @@
}
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.",
- name,
+ ai_choice_rule_name(&pcity->ai.choice),
pcity->ai.choice.want);
/* parallel to citytools change_build_target() */
@@ -1541,7 +1527,6 @@
int buycost;
int limit = cached_limit; /* cached_limit is our gold reserve */
struct city *pcity = NULL;
- const char *name = "(unknown)";
/* Find highest wanted item on the buy list */
init_choice(&bestchoice);
@@ -1627,20 +1612,6 @@
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 */
@@ -1651,7 +1622,7 @@
|| (bestchoice.want > 200 && pcity->ai.urgency > 1))) {
/* Buy stuff */
CITY_LOG(LOG_BUY, pcity, "Crash buy of %s for %d (want %d)",
- name,
+ ai_choice_rule_name(&bestchoice),
buycost,
bestchoice.want);
really_handle_city_buy(pplayer, pcity);
@@ -1660,8 +1631,9 @@
&& 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)!",
- name,
- pplayer->economic.gold, buycost);
+ ai_choice_rule_name(&bestchoice),
+ pplayer->economic.gold,
+ buycost);
try_to_sell_stuff(pplayer, pcity);
if (pplayer->economic.gold - pplayer->ai.est_upkeep >= buycost) {
CITY_LOG(LOG_BUY, pcity, "now we can afford it (sold something)");
Index: ai/advmilitary.c
===================================================================
--- ai/advmilitary.c (revision 13913)
+++ ai/advmilitary.c (working copy)
@@ -720,8 +720,8 @@
memset(tech_desire, 0, sizeof(tech_desire));
simple_ai_unit_type_iterate(punittype) {
- int move_type = utype_move_type(punittype);
int desire; /* How much we want the unit? */
+ int move_type = utype_move_type(punittype);
/* Only consider proper defenders - otherwise waste CPU and
* bump tech want needlessly. */
@@ -885,15 +885,9 @@
}
simple_ai_unit_type_iterate(punittype) {
- Tech_type_id tech_req;
- int tech_dist;
+ Tech_type_id tech_req = advance_number(punittype->require_advance);
+ int tech_dist = num_unknown_techs_for_goal(pplayer, tech_req);
int move_type = utype_move_type(punittype);
-
- if (A_NEVER == punittype->require_advance) {
- continue;
- }
- tech_req = advance_number(punittype->require_advance);
- tech_dist = num_unknown_techs_for_goal(pplayer, tech_req);
if ((move_type == LAND_MOVING || (move_type == SEA_MOVING && shore))
&& (tech_dist > 0
@@ -913,8 +907,7 @@
/* Cost (shield equivalent) of gaining these techs. */
/* FIXME? Katvrr advises that this should be weighted more heavily in big
* danger. */
- int tech_cost = total_bulbs_required_for_goal(pplayer,
- advance_number(punittype->require_advance)) / 4
+ int tech_cost = total_bulbs_required_for_goal(pplayer, tech_req) / 4
/ city_list_size(pplayer->cities);
int move_rate = punittype->move_rate;
int bcost_balanced = build_cost_balanced(punittype);
@@ -923,7 +916,6 @@
int attack = unittype_att_rating(punittype, will_be_veteran,
SINGLE_MOVE,
punittype->hp);
- struct impr_type *impr_req = punittype->need_improvement;
/* Take into account reinforcements strength */
if (acity) attack += acity->ai.attack;
@@ -1012,6 +1004,8 @@
(acity ? acity->name : utype_rule_name(victim_unit_type)),
TILE_XY(ptile));
} else if (want > best_choice->want) {
+ struct impr_type *impr_req = punittype->need_improvement;
+
if (can_city_build_unit_now(pcity, punittype)) {
/* This is a real unit and we really want it */
@@ -1026,11 +1020,14 @@
best_choice->value.utype = punittype;
best_choice->want = want;
best_choice->type = CT_ATTACKER;
+ } else if (NULL == impr_req) {
+ CITY_LOG(LOG_DEBUG, pcity, "cannot build unit %s",
+ utype_rule_name(punittype));
} else if (can_city_build_improvement_now(pcity, impr_req)) {
/* Building this unit requires a specific type of improvement.
* So we build this improvement instead. This may not be the
* best behavior. */
- CITY_LOG(LOG_DEBUG, pcity, "building %s to build %s",
+ CITY_LOG(LOG_DEBUG, pcity, "building %s to build unit %s",
improvement_rule_name(impr_req),
utype_rule_name(punittype));
best_choice->value.building = impr_req;
@@ -1038,13 +1035,9 @@
best_choice->type = CT_BUILDING;
} else {
/* This should never happen? */
- /* FIXME: Restore this when bug causing it to crash is fixed.
- * Commented out just as a critical fix to avoid crash. */
-#if 0
- CITY_LOG(LOG_DEBUG, pcity, "cannot build %s or %s",
+ CITY_LOG(LOG_DEBUG, pcity, "cannot build %s or unit %s",
improvement_rule_name(impr_req),
utype_rule_name(punittype));
-#endif
}
}
}
@@ -1492,23 +1485,8 @@
if (choice->want <= 0) {
CITY_LOG(LOGLEVEL_BUILD, pcity, "military advisor has no advice");
} 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)",
- name,
+ ai_choice_rule_name(choice),
choice->want);
}
}
Index: ai/aiunit.c
===================================================================
--- ai/aiunit.c (revision 13913)
+++ ai/aiunit.c (working copy)
@@ -2499,7 +2499,8 @@
int i = 0;
unit_type_iterate(punittype) {
- if (!utype_has_flag(punittype, F_CIVILIAN)
+ if (A_NEVER != punittype->require_advance
+ && !utype_has_flag(punittype, F_CIVILIAN)
&& !uclass_has_flag(utype_class(punittype), UCF_MISSILE)
&& !utype_has_flag(punittype, F_NO_LAND_ATTACK)
&& utype_move_type(punittype) != AIR_MOVING
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
