From dd4bdca72bc4ab98806fa492525195f86c64bb31 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Sat, 6 May 2017 15:05:36 +0200 Subject: [PATCH 1/6] do not use the shiptypes list for searches, use a cbtrie. --- src/kernel/ship.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/kernel/ship.c b/src/kernel/ship.c index b9ac0b170..518c3ce0c 100644 --- a/src/kernel/ship.c +++ b/src/kernel/ship.c @@ -37,13 +37,15 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. #include #include #include +#include #include -#include #include #include #include +#include +#include /* libc includes */ #include @@ -52,6 +54,7 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. #include selist *shiptypes = NULL; +static critbit_tree cb_shiptypes; static local_names *snames; @@ -89,16 +92,17 @@ const ship_type *findshiptype(const char *name, const struct locale *lang) static ship_type *st_find_i(const char *name) { - selist *ql; - int qi; + const char *match; + ship_type *st = NULL; - for (qi = 0, ql = shiptypes; ql; selist_advance(&ql, &qi, 1)) { - ship_type *stype = (ship_type *)selist_get(ql, qi); - if (strcmp(stype->_name, name) == 0) { - return stype; - } + match = cb_find_str(&cb_shiptypes, name); + if (match) { + cb_get_kv(match, &st, sizeof(st)); } - return NULL; + else { + log_warning("st_find: could not find ship '%s'\n", name); + } + return st; } const ship_type *st_find(const char *name) { @@ -108,10 +112,17 @@ const ship_type *st_find(const char *name) { ship_type *st_get_or_create(const char * name) { ship_type * st = st_find_i(name); if (!st) { + size_t len; + char data[64]; + st = (ship_type *)calloc(sizeof(ship_type), 1); st->_name = strdup(name); st->storm = 1.0; selist_push(&shiptypes, (void *)st); + + len = cb_new_kv(name, strlen(name), &st, sizeof(st), data); + assert(len <= sizeof(data)); + cb_insert(&cb_shiptypes, data, len); } return st; } @@ -250,6 +261,7 @@ static void free_shiptype(void *ptr) { } void free_shiptypes(void) { + cb_clear(&cb_shiptypes); selist_foreach(shiptypes, free_shiptype); selist_free(shiptypes); shiptypes = 0; From 6778cbe4836a25ac268b2fd38f42cba759279509 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Sat, 6 May 2017 15:25:13 +0200 Subject: [PATCH 2/6] assert that we do not add new ship types after the per-language lookup is initialized. --- src/kernel/ship.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/kernel/ship.c b/src/kernel/ship.c index 518c3ce0c..88c0f31e7 100644 --- a/src/kernel/ship.c +++ b/src/kernel/ship.c @@ -53,8 +53,8 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. #include #include -selist *shiptypes = NULL; -static critbit_tree cb_shiptypes; +selist *shiptypes = NULL; /* do not use this list for searching*/ +static critbit_tree cb_shiptypes; /* use this trie instead */ static local_names *snames; @@ -63,9 +63,7 @@ const ship_type *findshiptype(const char *name, const struct locale *lang) local_names *sn = snames; variant var; - while (sn) { - if (sn->lang == lang) - break; + while (sn && sn->lang != lang) { sn = sn->next; } if (!sn) { @@ -111,6 +109,7 @@ const ship_type *st_find(const char *name) { ship_type *st_get_or_create(const char * name) { ship_type * st = st_find_i(name); + assert(!snames); if (!st) { size_t len; char data[64]; From 3eb89e93aecc903f1b4bfb2ae21b3d20945feafa Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Sat, 6 May 2017 15:33:35 +0200 Subject: [PATCH 3/6] use a cbtrie for shiptype-lookups instead of the selist. --- src/kernel/building.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/kernel/building.c b/src/kernel/building.c index bb97f57b8..1053a7748 100644 --- a/src/kernel/building.c +++ b/src/kernel/building.c @@ -41,11 +41,12 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. #include #include #include -#include #include #include #include +#include +#include /* libc includes */ #include @@ -62,21 +63,22 @@ typedef struct building_typelist { } building_typelist; selist *buildingtypes = NULL; +static critbit_tree cb_bldgtypes; /* Returns a building type for the (internal) name */ static building_type *bt_find_i(const char *name) { - selist *ql; - int qi; + const char *match; + building_type *btype = NULL; - assert(name); - - for (qi = 0, ql = buildingtypes; ql; selist_advance(&ql, &qi, 1)) { - building_type *btype = (building_type *)selist_get(ql, qi); - if (strcmp(btype->_name, name) == 0) - return btype; + match = cb_find_str(&cb_bldgtypes, name); + if (match) { + cb_get_kv(match, &btype, sizeof(btype)); } - return NULL; + else { + log_warning("st_find: could not find ship '%s'\n", name); + } + return btype; } const building_type *bt_find(const char *name) @@ -96,9 +98,15 @@ bool bt_changed(int *cache) return false; } -void bt_register(building_type * type) +void bt_register(building_type * btype) { - selist_push(&buildingtypes, (void *)type); + size_t len; + char data[64]; + + selist_push(&buildingtypes, (void *)btype); + len = cb_new_kv(btype->_name, strlen(btype->_name), &btype, sizeof(btype), data); + assert(len <= sizeof(data)); + cb_insert(&cb_bldgtypes, data, len); ++bt_changes; } @@ -111,6 +119,7 @@ static void free_buildingtype(void *ptr) { } void free_buildingtypes(void) { + cb_clear(&cb_bldgtypes); selist_foreach(buildingtypes, free_buildingtype); selist_free(buildingtypes); buildingtypes = 0; From b74d18b8c93d74659fc1662d2086d04300f1d565 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Sat, 6 May 2017 15:39:09 +0200 Subject: [PATCH 4/6] bt_register is non-standard API, hide it. factor out st_register for readability. --- src/kernel/building.c | 2 +- src/kernel/building.h | 1 - src/kernel/building.test.c | 9 ++++----- src/kernel/ship.c | 20 ++++++++++++-------- src/market.test.c | 4 +--- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/kernel/building.c b/src/kernel/building.c index 1053a7748..948797b94 100644 --- a/src/kernel/building.c +++ b/src/kernel/building.c @@ -98,7 +98,7 @@ bool bt_changed(int *cache) return false; } -void bt_register(building_type * btype) +static void bt_register(building_type * btype) { size_t len; char data[64]; diff --git a/src/kernel/building.h b/src/kernel/building.h index edb46edfd..a2019e4a2 100644 --- a/src/kernel/building.h +++ b/src/kernel/building.h @@ -83,7 +83,6 @@ extern "C" { bool bt_changed(int *cache); const building_type *bt_find(const char *name); void free_buildingtypes(void); - void bt_register(struct building_type *type); int bt_effsize(const struct building_type *btype, const struct building *b, int bsize); diff --git a/src/kernel/building.test.c b/src/kernel/building.test.c index 3d25cd9a1..39b145d24 100644 --- a/src/kernel/building.test.c +++ b/src/kernel/building.test.c @@ -21,14 +21,13 @@ static void test_register_building(CuTest * tc) test_cleanup(); - btype = (building_type *)calloc(sizeof(building_type), 1); - btype->_name = strdup("herp"); CuAssertIntEquals(tc, true, bt_changed(&cache)); CuAssertIntEquals(tc, false, bt_changed(&cache)); - bt_register(btype); - CuAssertIntEquals(tc, true, bt_changed(&cache)); - CuAssertPtrNotNull(tc, bt_find("herp")); + btype = bt_get_or_create("herp"); + CuAssertIntEquals(tc, true, bt_changed(&cache)); + CuAssertPtrEquals(tc, btype, (void *)bt_find("herp")); + free_buildingtypes(); CuAssertIntEquals(tc, true, bt_changed(&cache)); test_cleanup(); diff --git a/src/kernel/ship.c b/src/kernel/ship.c index 88c0f31e7..27db2e5c2 100644 --- a/src/kernel/ship.c +++ b/src/kernel/ship.c @@ -107,21 +107,25 @@ const ship_type *st_find(const char *name) { return st_find_i(name); } +static void st_register(ship_type *stype) { + size_t len; + char data[64]; + + selist_push(&shiptypes, (void *)stype); + + len = cb_new_kv(stype->_name, strlen(stype->_name), &stype, sizeof(stype), data); + assert(len <= sizeof(data)); + cb_insert(&cb_shiptypes, data, len); +} + ship_type *st_get_or_create(const char * name) { ship_type * st = st_find_i(name); assert(!snames); if (!st) { - size_t len; - char data[64]; - st = (ship_type *)calloc(sizeof(ship_type), 1); st->_name = strdup(name); st->storm = 1.0; - selist_push(&shiptypes, (void *)st); - - len = cb_new_kv(name, strlen(name), &st, sizeof(st), data); - assert(len <= sizeof(data)); - cb_insert(&cb_shiptypes, data, len); + st_register(st); } return st; } diff --git a/src/market.test.c b/src/market.test.c index a631dbcb7..2beeaea6b 100644 --- a/src/market.test.c +++ b/src/market.test.c @@ -43,9 +43,7 @@ static void test_market_curse(CuTest * tc) config_set("rules.region_owners", "1"); - btype = (building_type *)calloc(1, sizeof(building_type)); - btype->_name = strdup("market"); - bt_register(btype); + btype = bt_get_or_create("market"); terrain = get_terrain("plain"); From 898c12e99a89d06839e0e5a798776555b0d55561 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Sat, 6 May 2017 15:53:21 +0200 Subject: [PATCH 5/6] XML construction elements never have a building. required buildings are encoded by RMT_PROD_REQUIRE. improved error messaging. --- scripts/tests/production.lua | 2 +- src/economy.c | 24 ++++++++++-------------- src/economy.test.c | 2 +- src/kernel/build.c | 11 ----------- src/kernel/build.h | 10 ++-------- src/kernel/build.test.c | 24 ------------------------ src/kernel/building.c | 4 ++-- src/kernel/building.test.c | 2 +- src/kernel/ship.c | 3 ++- src/kernel/xmlreader.c | 11 ++--------- src/magic.test.c | 2 +- 11 files changed, 22 insertions(+), 73 deletions(-) diff --git a/scripts/tests/production.lua b/scripts/tests/production.lua index 8727762d1..c8a299676 100644 --- a/scripts/tests/production.lua +++ b/scripts/tests/production.lua @@ -27,7 +27,7 @@ function test_laen_needs_mine() turn_process() assert_equal(0, u:get_item('laen')) assert_equal(100, r:get_resource('laen')) - assert_equal(1, f:count_msg_type("error104")) -- requires building + assert_equal(1, f:count_msg_type("building_needed")) -- requires building u.building = building.create(u.region, "mine") u.building.working = true diff --git a/src/economy.c b/src/economy.c index 48653d0aa..5f62527ea 100644 --- a/src/economy.c +++ b/src/economy.c @@ -814,6 +814,7 @@ static struct message * get_modifiers(unit *u, skill_t sk, const resource_type * int skill = 0; int need_race = 0, need_bldg = 0; resource_mod *mod; + const struct building_type *btype_needed = NULL; if (btype && btype->modifiers) { for (mod = btype->modifiers; mod && mod->type != RMT_END; ++mod) { @@ -838,7 +839,9 @@ static struct message * get_modifiers(unit *u, skill_t sk, const resource_type * break; case RMT_PROD_REQUIRE: if (mod->race) need_race |= 1; - if (mod->btype) need_bldg |= 1; + if (mod->btype) { + need_bldg |= 1; + } break; default: /* is not a production modifier, ignore it */ @@ -848,14 +851,17 @@ static struct message * get_modifiers(unit *u, skill_t sk, const resource_type * } if (mod->type == RMT_PROD_REQUIRE) { if (mod->race) need_race |= 2; - if (mod->btype) need_bldg |= 2; + if (mod->btype) { + btype_needed = mod->btype; + need_bldg |= 2; + } } } if (need_race == 2) { return msg_error(u, u->thisorder, 117); } - if (need_bldg == 2) { - return msg_error(u, u->thisorder, 104); + if (btype_needed && need_bldg == 2) { + return msg_feedback(u, u->thisorder, "building_needed", "building", btype_needed->_name); } *skillp = skill; if (savep) *savep = frac_make(save_n, save_d); @@ -885,11 +891,6 @@ static void manufacture(unit * u, const item_type * itype, int want) ADDMSG(&u->faction->msgs, msg_feedback(u, u->thisorder, "skill_needed", "skill", sk)); return; - case EBUILDINGREQ: - ADDMSG(&u->faction->msgs, - msg_feedback(u, u->thisorder, "building_needed", "building", - itype->construction->extra.btype->_name)); - return; case ELOWSKILL: ADDMSG(&u->faction->msgs, msg_feedback(u, u->thisorder, "manufacture_skills", @@ -1236,11 +1237,6 @@ static void create_potion(unit * u, const potion_type * ptype, int want) /* no skill, or not enough skill points to build */ cmistake(u, u->thisorder, 50, MSG_PRODUCE); break; - case EBUILDINGREQ: - ADDMSG(&u->faction->msgs, - msg_feedback(u, u->thisorder, "building_needed", "building", - ptype->itype->construction->extra.btype->_name)); - break; case ECOMPLETE: assert(0); break; diff --git a/src/economy.test.c b/src/economy.test.c index 842db45f8..802610880 100644 --- a/src/economy.test.c +++ b/src/economy.test.c @@ -527,7 +527,7 @@ static void test_modify_production(CuTest *tc) { test_clear_messages(u->faction); make_item(u, itype, 10); CuAssertIntEquals(tc, 28, get_item(u, itype)); - CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error104")); + CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "building_needed")); rtype->modifiers[0].type = RMT_PROD_REQUIRE; rtype->modifiers[0].race = test_create_race("smurf"); diff --git a/src/kernel/build.c b/src/kernel/build.c index 36d72b109..57c172905 100644 --- a/src/kernel/build.c +++ b/src/kernel/build.c @@ -513,17 +513,6 @@ int build(unit * u, const construction * ctype, int completed, int want, int ski return ENOMATERIALS; if (con->improvement == NULL && completed == con->maxsize) return ECOMPLETE; - if (con->type==CONS_ITEM && con->extra.btype) { - building *b; - if (!u->building || u->building->type != con->extra.btype) { - return EBUILDINGREQ; - } - b = inside_building(u); - if (!b || !building_is_active(b)) { - return EBUILDINGREQ; - } - } - if (con->skill != NOSKILL) { int effsk; int dm = get_effect(u, oldpotiontype[P_DOMORE]); diff --git a/src/kernel/build.h b/src/kernel/build.h index 39916365a..c201d900b 100644 --- a/src/kernel/build.h +++ b/src/kernel/build.h @@ -52,13 +52,8 @@ extern "C" { int reqsize; /* size of object using up 1 set of requirement. */ requirement *materials; /* material req'd to build one object */ - union { - /* CONS_BUILDING: */ - char * name; /* building level name */ - /* CONS_ITEM: */ - const struct building_type *btype; /* building required to build item */ - } extra; - + /* only used by CONS_BUILDING: */ + char * name; /* building level name */ struct construction *improvement; /* next level, if upgradable. */ } construction; @@ -89,7 +84,6 @@ extern "C" { #define ENEEDSKILL -2 #define ECOMPLETE -3 #define ENOMATERIALS -4 -#define EBUILDINGREQ -5 #ifdef __cplusplus } diff --git a/src/kernel/build.test.c b/src/kernel/build.test.c index c3fa07e65..8cd408b09 100644 --- a/src/kernel/build.test.c +++ b/src/kernel/build.test.c @@ -71,29 +71,6 @@ static void test_build_requires_materials(CuTest *tc) { teardown_build(&bf); } -static void test_build_requires_building(CuTest *tc) { - build_fixture bf = { 0 }; - unit *u; - const struct resource_type *rtype; - building_type *btype; - - u = setup_build(&bf); - rtype = bf.cons.materials[0].rtype; - i_change(&u->items, rtype->itype, 1); - set_level(u, SK_ARMORER, 2); - bf.cons.type = CONS_ITEM; - bf.cons.extra.btype = btype = bt_get_or_create("hodor"); - btype->maxcapacity = 1; - btype->capacity = 1; - CuAssertIntEquals_Msg(tc, "must be inside a production building", EBUILDINGREQ, build(u, &bf.cons, 0, 1, 0)); - u->building = test_create_building(u->region, btype); - fset(u->building, BLD_MAINTAINED); - CuAssertIntEquals(tc, 1, build(u, &bf.cons, 0, 1, 0)); - btype->maxcapacity = 0; - CuAssertIntEquals_Msg(tc, "cannot build when production building capacity exceeded", EBUILDINGREQ, build(u, &bf.cons, 0, 1, 0)); - teardown_build(&bf); -} - static void test_build_failure_missing_skill(CuTest *tc) { build_fixture bf = { 0 }; unit *u; @@ -411,7 +388,6 @@ CuSuite *get_build_suite(void) SUITE_ADD_TEST(suite, test_build_failure_low_skill); SUITE_ADD_TEST(suite, test_build_failure_missing_skill); SUITE_ADD_TEST(suite, test_build_requires_materials); - SUITE_ADD_TEST(suite, test_build_requires_building); SUITE_ADD_TEST(suite, test_build_failure_completed); SUITE_ADD_TEST(suite, test_build_with_ring); SUITE_ADD_TEST(suite, test_build_with_potion); diff --git a/src/kernel/building.c b/src/kernel/building.c index 948797b94..31c4b3e27 100644 --- a/src/kernel/building.c +++ b/src/kernel/building.c @@ -199,14 +199,14 @@ const char *buildingtype(const building_type * btype, const building * b, int bs if (btype->name) { return btype->name(btype, b, bsize); } - if (btype->construction && btype->construction->extra.name) { + if (btype->construction && btype->construction->name) { if (b) { bsize = adjust_size(b, bsize); } for (con = btype->construction; con; con = con->improvement) { bsize -= con->maxsize; if (!con->improvement || bsize <0) { - return con->extra.name; + return con->name; } } } diff --git a/src/kernel/building.test.c b/src/kernel/building.test.c index 39b145d24..c1255373b 100644 --- a/src/kernel/building.test.c +++ b/src/kernel/building.test.c @@ -563,7 +563,7 @@ static void test_buildingtype(CuTest *tc) { btype = test_create_buildingtype("hodor"); CuAssertPtrNotNull(tc, btype->construction); CuAssertStrEquals(tc, "hodor", buildingtype(btype, NULL, 1)); - btype->construction->extra.name = strdup("castle"); + btype->construction->name = strdup("castle"); CuAssertStrEquals(tc, "castle", buildingtype(btype, NULL, 1)); btype = bt_get_or_create("portal"); CuAssertPtrEquals(tc, NULL, btype->construction); diff --git a/src/kernel/ship.c b/src/kernel/ship.c index 27db2e5c2..3a7608789 100644 --- a/src/kernel/ship.c +++ b/src/kernel/ship.c @@ -248,8 +248,9 @@ void remove_ship(ship ** slist, ship * sh) void free_ship(ship * s) { - while (s->attribs) + while (s->attribs) { a_remove(&s->attribs, s->attribs); + } free(s->name); free(s->display); free(s); diff --git a/src/kernel/xmlreader.c b/src/kernel/xmlreader.c index 9f671d44c..080ff4207 100644 --- a/src/kernel/xmlreader.c +++ b/src/kernel/xmlreader.c @@ -247,17 +247,10 @@ construction ** consPtr, construct_t type) con->minskill = xml_ivalue(node, "minskill", -1); con->reqsize = xml_ivalue(node, "reqsize", 1); - if (type == CONS_ITEM) { - propValue = xmlGetProp(node, BAD_CAST "building"); - if (propValue != NULL) { - con->extra.btype = bt_get_or_create((const char *)propValue); - xmlFree(propValue); - } - } - else if (type == CONS_BUILDING) { + if (type == CONS_BUILDING) { propValue = xmlGetProp(node, BAD_CAST "name"); if (propValue != NULL) { - con->extra.name = strdup((const char *)propValue); + con->name = strdup((const char *)propValue); xmlFree(propValue); } } diff --git a/src/magic.test.c b/src/magic.test.c index 380001035..a1b55b45d 100644 --- a/src/magic.test.c +++ b/src/magic.test.c @@ -479,7 +479,7 @@ static void test_illusioncastle(CuTest *tc) CuAssertPtrEquals(tc, btype, (void *)icastle_type(a)); CuAssertPtrEquals(tc, bt_icastle, (void *)b->type); CuAssertStrEquals(tc, "castle", buildingtype(btype, b, b->size)); - btype->construction->extra.name = strdup("site"); + btype->construction->name = strdup("site"); CuAssertStrEquals(tc, "site", buildingtype(btype, b, b->size)); test_cleanup(); } From 9d5369ff1efc17a53f13c11dd95ac261f3df74c8 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Sat, 6 May 2017 16:48:32 +0200 Subject: [PATCH 6/6] coverity: unintended integer division --- src/economy.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/economy.c b/src/economy.c index 5f62527ea..0bc056b56 100644 --- a/src/economy.c +++ b/src/economy.c @@ -2944,8 +2944,8 @@ static void peasant_taxes(region * r) level = buildingeffsize(b, false); if (level > 0) { - double taxfactor = money * level / building_taxes(b); - double morale = money * region_get_morale(r) / MORALE_TAX_FACTOR; + double taxfactor = (double)money * level / building_taxes(b); + double morale = (double)money * region_get_morale(r) / MORALE_TAX_FACTOR; if (taxfactor > morale) { taxfactor = morale; }