From 521519805d89e1179578b071172397d43567de29 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Sun, 11 Oct 2015 21:19:38 +0200 Subject: [PATCH 01/10] refactor piracy code into a module, separate from move. --- src/CMakeLists.txt | 1 + src/move.c | 171 ++--------------------------------------- src/move.h | 2 + src/piracy.c | 186 +++++++++++++++++++++++++++++++++++++++++++++ src/piracy.h | 21 +++++ 5 files changed, 218 insertions(+), 163 deletions(-) create mode 100644 src/piracy.c create mode 100644 src/piracy.h diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 10d4b2597..3309a8a28 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -79,6 +79,7 @@ ENDIF() set (ERESSEA_SRC calendar.c move.c + piracy.c spells.c battle.c alchemy.c diff --git a/src/move.c b/src/move.c index f708bfaa7..6d296eb81 100644 --- a/src/move.c +++ b/src/move.c @@ -27,6 +27,7 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. #include "vortex.h" #include "monster.h" #include "lighthouse.h" +#include "piracy.h" #include #include @@ -55,7 +56,6 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. #include #include #include -#include #include #include #include @@ -68,7 +68,6 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. /* attributes includes */ #include #include -#include #include #include @@ -1770,7 +1769,7 @@ buildingtype_exists(const region * r, const building_type * bt, bool working) /* Prüft, ob Ablegen von einer Küste in eine der erlaubten Richtungen erfolgt. */ -static bool check_takeoff(ship * sh, region * from, region * to) +bool can_takeoff(const ship * sh, const region * from, const region * to) { if (!fval(from->terrain, SEA_REGION) && sh->coast != NODIRECTION) { direction_t coast = sh->coast; @@ -1918,7 +1917,7 @@ sail(unit * u, order * ord, bool move_on_land, region_list ** routep) } } else { - if (!check_takeoff(sh, current_point, next_point)) { + if (!can_takeoff(sh, current_point, next_point)) { /* Schiff kann nicht ablegen */ cmistake(u, ord, 182, MSG_MOVE); break; @@ -2271,7 +2270,7 @@ static void travel(unit * u, region_list ** routep) } -static void move(unit * u, bool move_on_land) +void move_cmd(unit * u, bool move_on_land) { region_list *route = NULL; @@ -2290,160 +2289,6 @@ static void move(unit * u, bool move_on_land) free_regionlist(route); } -typedef struct piracy_data { - const struct faction *pirate; - const struct faction *target; - direction_t dir; -} piracy_data; - -static void piracy_init(struct attrib *a) -{ - a->data.v = calloc(1, sizeof(piracy_data)); -} - -static void piracy_done(struct attrib *a) -{ - free(a->data.v); -} - -static attrib_type at_piracy_direction = { - "piracy_direction", - piracy_init, - piracy_done, - DEFAULT_AGE, - NO_WRITE, - NO_READ -}; - -static attrib *mk_piracy(const faction * pirate, const faction * target, - direction_t target_dir) -{ - attrib *a = a_new(&at_piracy_direction); - piracy_data *data = a->data.v; - data->pirate = pirate; - data->target = target; - data->dir = target_dir; - return a; -} - -static void piracy_cmd(unit * u, struct order *ord) -{ - region *r = u->region; - ship *sh = u->ship, *sh2; - direction_t dir, target_dir = NODIRECTION; - struct { - const faction *target; - int value; - } aff[MAXDIRECTIONS]; - int saff = 0; - int *il = NULL; - const char *s; - attrib *a; - - if (!sh) { - cmistake(u, ord, 144, MSG_MOVE); - return; - } - - if (!u->ship || u != ship_owner(u->ship)) { - cmistake(u, ord, 146, MSG_MOVE); - return; - } - - /* Feststellen, ob schon ein anderer alliierter Pirat ein - * Ziel gefunden hat. */ - - init_order(ord); - s = getstrtoken(); - if (s != NULL && *s) { - il = intlist_init(); - while (s && *s) { - il = intlist_add(il, atoi36(s)); - s = getstrtoken(); - } - } - - for (a = a_find(r->attribs, &at_piracy_direction); - a && a->type == &at_piracy_direction; a = a->next) { - piracy_data *data = a->data.v; - const faction *p = data->pirate; - const faction *t = data->target; - - if (alliedunit(u, p, HELP_FIGHT)) { - if (il == 0 || (t && intlist_find(il, t->no))) { - target_dir = data->dir; - break; - } - } - } - - /* Wenn nicht, sehen wir, ob wir ein Ziel finden. */ - - if (target_dir == NODIRECTION) { - /* Einheit ist also Kapitän. Jetzt gucken, in wievielen - * Nachbarregionen potentielle Opfer sind. */ - - for (dir = 0; dir < MAXDIRECTIONS; dir++) { - region *rc = rconnect(r, dir); - aff[dir].value = 0; - aff[dir].target = 0; - if (rc && fval(rc->terrain, SWIM_INTO) && check_takeoff(sh, r, rc)) { - - for (sh2 = rc->ships; sh2; sh2 = sh2->next) { - unit *cap = ship_owner(sh2); - if (cap) { - faction *f = visible_faction(cap->faction, cap); - if (alliedunit(u, f, HELP_FIGHT)) - continue; - if (il == 0 || intlist_find(il, cap->faction->no)) { - ++aff[dir].value; - if (rng_int() % aff[dir].value == 0) { - aff[dir].target = f; - } - } - } - } - - /* Und aufaddieren. */ - saff += aff[dir].value; - } - } - - if (saff != 0) { - saff = rng_int() % saff; - for (dir = 0; dir != MAXDIRECTIONS; ++dir) { - if (saff < aff[dir].value) - break; - saff -= aff[dir].value; - } - target_dir = dir; - a = - a_add(&r->attribs, mk_piracy(u->faction, aff[dir].target, target_dir)); - } - } - - free(il); - - /* Wenn kein Ziel gefunden, entsprechende Meldung generieren */ - if (target_dir == NODIRECTION) { - ADDMSG(&u->faction->msgs, msg_message("piratenovictim", - "ship region", sh, r)); - return; - } - - /* Meldung generieren */ - ADDMSG(&u->faction->msgs, msg_message("piratesawvictim", - "ship region dir", sh, r, target_dir)); - - /* Befehl konstruieren */ - set_order(&u->thisorder, create_order(K_MOVE, u->faction->locale, "%s", - LOC(u->faction->locale, directions[target_dir]))); - - /* Bewegung ausführen */ - init_order(u->thisorder); - move(u, true); -} - static void age_traveldir(region * r) { attrib *a = a_find(r->attribs, &at_traveldir); @@ -2549,7 +2394,7 @@ static int follow_ship(unit * u, order * ord) init_tokens_str(command); getstrtoken(); /* NACH ausführen */ - move(u, false); + move_cmd(u, false); return 1; /* true -> Einheitenliste von vorne durchgehen */ } @@ -2656,7 +2501,7 @@ static void move_pirates(void) /* else *up is already the next unit */ } - a_removeall(&r->attribs, &at_piracy_direction); + age_piracy(r); age_traveldir(r); } } @@ -2716,13 +2561,13 @@ void movement(void) if (ships) { if (u->ship && ship_owner(u->ship) == u) { init_order(u->thisorder); - move(u, false); + move_cmd(u, false); } } else { if (!u->ship || ship_owner(u->ship) != u) { init_order(u->thisorder); - move(u, false); + move_cmd(u, false); } } } diff --git a/src/move.h b/src/move.h index 013885564..c0b92d071 100644 --- a/src/move.h +++ b/src/move.h @@ -74,6 +74,8 @@ extern "C" { const struct building_type *bt); bool move_blocked(const struct unit *u, const struct region *src, const struct region *dest); + bool can_takeoff(const struct ship * sh, const struct region * from, const struct region * to); + void move_cmd(struct unit * u, bool move_on_land); #define SA_HARBOUR 2 #define SA_COAST 1 diff --git a/src/piracy.c b/src/piracy.c new file mode 100644 index 000000000..0966317d5 --- /dev/null +++ b/src/piracy.c @@ -0,0 +1,186 @@ +#include +#include +#include "piracy.h" + +#include "direction.h" +#include "keyword.h" +#include "move.h" + +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +#include + +#include +#include + +typedef struct piracy_data { + const struct faction *pirate; + const struct faction *target; + direction_t dir; +} piracy_data; + +static void piracy_init(struct attrib *a) +{ + a->data.v = calloc(1, sizeof(piracy_data)); +} + +static void piracy_done(struct attrib *a) +{ + free(a->data.v); +} + +static attrib_type at_piracy_direction = { + "piracy_direction", + piracy_init, + piracy_done, + DEFAULT_AGE, + NO_WRITE, + NO_READ +}; + +static attrib *mk_piracy(const faction * pirate, const faction * target, + direction_t target_dir) +{ + attrib *a = a_new(&at_piracy_direction); + piracy_data *data = a->data.v; + data->pirate = pirate; + data->target = target; + data->dir = target_dir; + return a; +} + +void piracy_cmd(unit * u, struct order *ord) +{ + region *r = u->region; + ship *sh = u->ship, *sh2; + direction_t dir, target_dir = NODIRECTION; + struct { + const faction *target; + int value; + } aff[MAXDIRECTIONS]; + int saff = 0; + int *il = NULL; + const char *s; + attrib *a; + + if (!sh) { + cmistake(u, ord, 144, MSG_MOVE); + return; + } + + if (!u->ship || u != ship_owner(u->ship)) { + cmistake(u, ord, 146, MSG_MOVE); + return; + } + + /* Feststellen, ob schon ein anderer alliierter Pirat ein + * Ziel gefunden hat. */ + + init_order(ord); + s = getstrtoken(); + if (s != NULL && *s) { + il = intlist_init(); + while (s && *s) { + il = intlist_add(il, atoi36(s)); + s = getstrtoken(); + } + } + + for (a = a_find(r->attribs, &at_piracy_direction); + a && a->type == &at_piracy_direction; a = a->next) { + piracy_data *data = a->data.v; + const faction *p = data->pirate; + const faction *t = data->target; + + if (alliedunit(u, p, HELP_FIGHT)) { + if (il == 0 || (t && intlist_find(il, t->no))) { + target_dir = data->dir; + break; + } + } + } + + /* Wenn nicht, sehen wir, ob wir ein Ziel finden. */ + + if (target_dir == NODIRECTION) { + /* Einheit ist also Kapitän. Jetzt gucken, in wievielen + * Nachbarregionen potentielle Opfer sind. */ + + for (dir = 0; dir < MAXDIRECTIONS; dir++) { + region *rc = rconnect(r, dir); + aff[dir].value = 0; + aff[dir].target = 0; + if (rc && fval(rc->terrain, SWIM_INTO) && can_takeoff(sh, r, rc)) { + + for (sh2 = rc->ships; sh2; sh2 = sh2->next) { + unit *cap = ship_owner(sh2); + if (cap) { + faction *f = visible_faction(cap->faction, cap); + if (alliedunit(u, f, HELP_FIGHT)) + continue; + if (il == 0 || intlist_find(il, cap->faction->no)) { + ++aff[dir].value; + if (rng_int() % aff[dir].value == 0) { + aff[dir].target = f; + } + } + } + } + + /* Und aufaddieren. */ + saff += aff[dir].value; + } + } + + if (saff != 0) { + saff = rng_int() % saff; + for (dir = 0; dir != MAXDIRECTIONS; ++dir) { + if (saff < aff[dir].value) + break; + saff -= aff[dir].value; + } + target_dir = dir; + a = + a_add(&r->attribs, mk_piracy(u->faction, aff[dir].target, target_dir)); + } + } + + free(il); + + /* Wenn kein Ziel gefunden, entsprechende Meldung generieren */ + if (target_dir == NODIRECTION) { + ADDMSG(&u->faction->msgs, msg_message("piratenovictim", + "ship region", sh, r)); + return; + } + + /* Meldung generieren */ + ADDMSG(&u->faction->msgs, msg_message("piratesawvictim", + "ship region dir", sh, r, target_dir)); + + /* Befehl konstruieren */ + set_order(&u->thisorder, create_order(K_MOVE, u->faction->locale, "%s", + LOC(u->faction->locale, directions[target_dir]))); + + /* Bewegung ausführen */ + init_order(u->thisorder); + move_cmd(u, true); +} + +void age_piracy(region *r) { + a_removeall(&r->attribs, &at_piracy_direction); +} diff --git a/src/piracy.h b/src/piracy.h new file mode 100644 index 000000000..b74fbd509 --- /dev/null +++ b/src/piracy.h @@ -0,0 +1,21 @@ +#pragma once + +#ifndef PIRACY_H +#define PIRACY_H + +#ifdef __cplusplus +extern "C" { +#endif + + struct unit; + struct order; + struct region; + + void piracy_cmd(struct unit * u, struct order *ord); + void age_piracy(struct region *r); + +#ifdef __cplusplus +} +#endif + +#endif From 678169d7d4a415bc75dab5ffe9f82e23b9c5e1aa Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Mon, 12 Oct 2015 11:54:59 +0200 Subject: [PATCH 02/10] add skeleton test suite for piracy --- src/CMakeLists.txt | 1 + src/piracy.test.c | 34 ++++++++++++++++++++++++++++++++++ src/test_eressea.c | 1 + 3 files changed, 36 insertions(+) create mode 100644 src/piracy.test.c diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 3309a8a28..c2551cc45 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -201,6 +201,7 @@ set(TESTS_SRC magic.test.c market.test.c move.test.c + piracy.test.c prefix.test.c skill.test.c spells.test.c diff --git a/src/piracy.test.c b/src/piracy.test.c new file mode 100644 index 000000000..144bb8c3d --- /dev/null +++ b/src/piracy.test.c @@ -0,0 +1,34 @@ +#include +#include "piracy.h" + +#include +#include +#include + +#include + +#include +#include +#include + +static void test_piracy_cmd(CuTest * tc) { + unit *u; + order *ord; + + test_cleanup(); + u = test_create_unit(test_create_faction(0), test_create_region(0, 0, 0)); + u->faction->locale = get_or_create_locale("de"); + ord = create_order(K_PIRACY, u->faction->locale, ""); + assert(u); + piracy_cmd(u, ord); + CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error144")); + test_cleanup(); +} + + +CuSuite *get_piracy_suite(void) +{ + CuSuite *suite = CuSuiteNew(); + SUITE_ADD_TEST(suite, test_piracy_cmd); + return suite; +} diff --git a/src/test_eressea.c b/src/test_eressea.c index d5ab547ed..ec39f3134 100644 --- a/src/test_eressea.c +++ b/src/test_eressea.c @@ -89,6 +89,7 @@ int RunAllTests(void) RUN_TESTS(suite, laws); RUN_TESTS(suite, market); RUN_TESTS(suite, move); + RUN_TESTS(suite, piracy); RUN_TESTS(suite, stealth); RUN_TESTS(suite, upkeep); RUN_TESTS(suite, vortex); From dcf6c28b0e3b969fd7c80e39db6498e6c198b1cb Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Mon, 12 Oct 2015 13:27:39 +0200 Subject: [PATCH 03/10] testing all the error cases for PIRACY --- src/alchemy.test.c | 16 +++++----------- src/give.test.c | 4 +--- src/piracy.test.c | 30 +++++++++++++++++++++++------- src/tests.c | 5 +++++ src/tests.h | 1 + 5 files changed, 35 insertions(+), 21 deletions(-) diff --git a/src/alchemy.test.c b/src/alchemy.test.c index 7c01be81b..7e2e5b399 100644 --- a/src/alchemy.test.c +++ b/src/alchemy.test.c @@ -4,7 +4,6 @@ #include "move.h" #include -#include #include #include #include @@ -39,16 +38,14 @@ static void test_herbsearch(CuTest * tc) herbsearch(u, INT_MAX); CuAssertPtrNotNull(tc, test_find_messagetype(f->msgs, "error59")); - free_messagelist(f->msgs); - f->msgs = 0; + test_clear_messages(f); set_level(u, SK_HERBALISM, 1); CuAssertPtrEquals(tc, u2, is_guarded(r, u, GUARD_PRODUCE)); herbsearch(u, INT_MAX); CuAssertPtrNotNull(tc, test_find_messagetype(f->msgs, "error70")); CuAssertPtrEquals(tc, 0, test_find_messagetype(f->msgs, "error59")); - free_messagelist(f->msgs); - f->msgs = 0; + test_clear_messages(f); guard(u2, GUARD_NONE); CuAssertPtrEquals(tc, 0, is_guarded(r, u, GUARD_PRODUCE)); @@ -57,8 +54,7 @@ static void test_herbsearch(CuTest * tc) CuAssertPtrNotNull(tc, test_find_messagetype(f->msgs, "error108")); CuAssertPtrEquals(tc, 0, test_find_messagetype(f->msgs, "error70")); CuAssertPtrEquals(tc, 0, test_find_messagetype(f->msgs, "error59")); - free_messagelist(f->msgs); - f->msgs = 0; + test_clear_messages(f); rsetherbtype(r, itype); CuAssertPtrEquals(tc, (void *)itype, (void *)rherbtype(r)); @@ -68,8 +64,7 @@ static void test_herbsearch(CuTest * tc) CuAssertPtrEquals(tc, 0, test_find_messagetype(f->msgs, "error108")); CuAssertPtrEquals(tc, 0, test_find_messagetype(f->msgs, "error70")); CuAssertPtrEquals(tc, 0, test_find_messagetype(f->msgs, "error59")); - free_messagelist(f->msgs); - f->msgs = 0; + test_clear_messages(f); rsetherbs(r, 100); CuAssertIntEquals(tc, 100, rherbs(r)); @@ -81,8 +76,7 @@ static void test_herbsearch(CuTest * tc) CuAssertPtrEquals(tc, 0, test_find_messagetype(f->msgs, "error108")); CuAssertPtrEquals(tc, 0, test_find_messagetype(f->msgs, "error70")); CuAssertPtrEquals(tc, 0, test_find_messagetype(f->msgs, "error59")); - free_messagelist(f->msgs); - f->msgs = 0; + test_clear_messages(f); test_cleanup(); } diff --git a/src/give.test.c b/src/give.test.c index a81278edd..c9e005b8f 100644 --- a/src/give.test.c +++ b/src/give.test.c @@ -7,7 +7,6 @@ #include #include #include -#include #include #include #include @@ -214,8 +213,7 @@ static void test_give_men_requires_contact(CuTest * tc) { _snprintf(cmd, sizeof(cmd), "%s ALLES PERSONEN", itoa36(env.dst->no)); ord = create_order(K_GIVE, env.f1->locale, cmd); - free_messagelist(env.f1->msgs); - env.f1->msgs = 0; + test_clear_messages(env.f1); give_cmd(env.src, ord); CuAssertPtrEquals(tc, 0, test_find_messagetype(env.f1->msgs, "give_person")); CuAssertPtrNotNull(tc, test_find_messagetype(env.f1->msgs, "feedback_no_contact")); diff --git a/src/piracy.test.c b/src/piracy.test.c index 144bb8c3d..bea1068d6 100644 --- a/src/piracy.test.c +++ b/src/piracy.test.c @@ -2,6 +2,7 @@ #include "piracy.h" #include +#include #include #include @@ -11,17 +12,32 @@ #include #include -static void test_piracy_cmd(CuTest * tc) { - unit *u; +static void test_piracy_cmd_errors(CuTest * tc) { + faction *f; + unit *u, *u2; order *ord; test_cleanup(); - u = test_create_unit(test_create_faction(0), test_create_region(0, 0, 0)); - u->faction->locale = get_or_create_locale("de"); - ord = create_order(K_PIRACY, u->faction->locale, ""); + u = test_create_unit(f = test_create_faction(0), test_create_region(0, 0, 0)); + f->locale = get_or_create_locale("de"); + ord = create_order(K_PIRACY, f->locale, ""); assert(u); piracy_cmd(u, ord); - CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error144")); + CuAssertPtrNotNullMsg(tc, "must be on a ship for PIRACY", test_find_messagetype(f->msgs, "error144")); + + u_set_ship(u, test_create_ship(u->region, 0)); + + u2 = test_create_unit(u->faction, u->region); + u_set_ship(u2, u->ship); + + test_clear_messages(f); + piracy_cmd(u2, ord); + CuAssertPtrNotNullMsg(tc, "must be owner for PIRACY", test_find_messagetype(f->msgs, "error146")); + + test_clear_messages(f); + piracy_cmd(u, ord); + CuAssertPtrNotNullMsg(tc, "must specify target for PIRACY", test_find_messagetype(f->msgs, "piratenovictim")); + test_cleanup(); } @@ -29,6 +45,6 @@ static void test_piracy_cmd(CuTest * tc) { CuSuite *get_piracy_suite(void) { CuSuite *suite = CuSuiteNew(); - SUITE_ADD_TEST(suite, test_piracy_cmd); + SUITE_ADD_TEST(suite, test_piracy_cmd_errors); return suite; } diff --git a/src/tests.c b/src/tests.c index 05e0e99d9..b8c5f06c1 100644 --- a/src/tests.c +++ b/src/tests.c @@ -279,6 +279,11 @@ struct message * test_find_messagetype(struct message_list *msgs, const char *na return 0; } +void test_clear_messages(faction *f) { + free_messagelist(f->msgs); + f->msgs = 0; +} + void disabled_test(void *suite, void (*test)(CuTest *), const char *name) { (void)test; fprintf(stderr, "%s: SKIP\n", name); diff --git a/src/tests.h b/src/tests.h index 27f5de2db..a458e4eab 100644 --- a/src/tests.h +++ b/src/tests.h @@ -43,6 +43,7 @@ extern "C" { const char * test_get_messagetype(const struct message *msg); struct message * test_find_messagetype(struct message_list *msgs, const char *name); struct message * test_get_last_message(struct message_list *mlist); + void test_clear_messages(struct faction *f); void disabled_test(void *suite, void (*)(struct CuTest *), const char *name); From 4c028bceaced7139fdb2cf4be0615565932cb7be Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Mon, 12 Oct 2015 16:30:56 +0200 Subject: [PATCH 04/10] the early beginnings of a test for piracy extract validation message from piracy_cmd fix a SAIL/SWIM check inconsistency --- src/move.c | 12 ++++++------ src/piracy.c | 26 ++++++++++++++++---------- src/piracy.test.c | 45 ++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 64 insertions(+), 19 deletions(-) diff --git a/src/move.c b/src/move.c index 6d296eb81..c71f45c4f 100644 --- a/src/move.c +++ b/src/move.c @@ -1719,15 +1719,15 @@ static bool ship_ready(const region * r, unit * u) u->ship)); return false; } - assert(u->ship->type->construction->improvement == NULL); /* sonst ist construction::size nicht ship_type::maxsize */ - if (u->ship->size != u->ship->type->construction->maxsize) { - cmistake(u, u->thisorder, 15, MSG_MOVE); - return false; + if (u->ship->type->construction) { + assert(!u->ship->type->construction->improvement); /* sonst ist construction::size nicht ship_type::maxsize */ + if (u->ship->size != u->ship->type->construction->maxsize) { + cmistake(u, u->thisorder, 15, MSG_MOVE); + return false; + } } if (!enoughsailors(u->ship, crew_skill(u->ship))) { cmistake(u, u->thisorder, 1, MSG_MOVE); - /* mistake(u, u->thisorder, - "Auf dem Schiff befinden sich zuwenig erfahrene Seeleute.", MSG_MOVE); */ return false; } if (!cansail(r, u->ship)) { diff --git a/src/piracy.c b/src/piracy.c index 0966317d5..93e4e346e 100644 --- a/src/piracy.c +++ b/src/piracy.c @@ -63,7 +63,20 @@ static attrib *mk_piracy(const faction * pirate, const faction * target, return a; } -void piracy_cmd(unit * u, struct order *ord) +static bool validate_pirate(unit *u, order *ord) { + if (!u->ship) { + cmistake(u, ord, 144, MSG_MOVE); + return false; + } + + if (!u->ship || u != ship_owner(u->ship)) { + cmistake(u, ord, 146, MSG_MOVE); + return false; + } + return true; +} + +void piracy_cmd(unit * u, order *ord) { region *r = u->region; ship *sh = u->ship, *sh2; @@ -77,19 +90,12 @@ void piracy_cmd(unit * u, struct order *ord) const char *s; attrib *a; - if (!sh) { - cmistake(u, ord, 144, MSG_MOVE); - return; - } - - if (!u->ship || u != ship_owner(u->ship)) { - cmistake(u, ord, 146, MSG_MOVE); + if (!validate_pirate(u, ord)) { return; } /* Feststellen, ob schon ein anderer alliierter Pirat ein * Ziel gefunden hat. */ - init_order(ord); s = getstrtoken(); if (s != NULL && *s) { @@ -124,7 +130,7 @@ void piracy_cmd(unit * u, struct order *ord) region *rc = rconnect(r, dir); aff[dir].value = 0; aff[dir].target = 0; - if (rc && fval(rc->terrain, SWIM_INTO) && can_takeoff(sh, r, rc)) { + if (rc && fval(rc->terrain, SAIL_INTO) && can_takeoff(sh, r, rc)) { for (sh2 = rc->ships; sh2; sh2 = sh2->next) { unit *cap = ship_owner(sh2); diff --git a/src/piracy.test.c b/src/piracy.test.c index bea1068d6..250fc8b10 100644 --- a/src/piracy.test.c +++ b/src/piracy.test.c @@ -3,25 +3,63 @@ #include #include +#include #include #include +#include #include #include #include #include +static void setup_piracy(void) { + struct locale *lang; + terrain_type *t_ocean; + + test_cleanup(); + lang = get_or_create_locale("de"); + locale_setstring(lang, directions[D_EAST], "OSTEN"); + init_directions(lang); + t_ocean = test_create_terrain("ocean", SAIL_INTO | SEA_REGION); +} + +static void test_piracy_cmd(CuTest * tc) { + faction *f; + unit *u, *u2; + order *ord; + terrain_type *t_ocean; + + setup_piracy(); + t_ocean = get_or_create_terrain("ocean"); + u2 = test_create_unit(test_create_faction(0), test_create_region(1, 0, t_ocean)); + u_set_ship(u2, test_create_ship(u2->region, 0)); + assert(u2); + u = test_create_unit(f = test_create_faction(0), test_create_region(0, 0, t_ocean)); + u_set_ship(u, test_create_ship(u->region, 0)); + assert(f && u); + f->locale = get_or_create_locale("de"); + ord = create_order(K_PIRACY, f->locale, "%s", itoa36(u2->faction->no)); + assert(ord); + + piracy_cmd(u, ord); + CuAssertPtrNotNullMsg(tc, "successful PIRACY", test_find_messagetype(f->msgs, "piratesawvictim")); + + test_cleanup(); +} + static void test_piracy_cmd_errors(CuTest * tc) { faction *f; unit *u, *u2; order *ord; - test_cleanup(); - u = test_create_unit(f = test_create_faction(0), test_create_region(0, 0, 0)); + setup_piracy(); + u = test_create_unit(f = test_create_faction(0), test_create_region(0, 0, get_or_create_terrain("ocean"))); f->locale = get_or_create_locale("de"); ord = create_order(K_PIRACY, f->locale, ""); - assert(u); + assert(u && ord); + piracy_cmd(u, ord); CuAssertPtrNotNullMsg(tc, "must be on a ship for PIRACY", test_find_messagetype(f->msgs, "error144")); @@ -46,5 +84,6 @@ CuSuite *get_piracy_suite(void) { CuSuite *suite = CuSuiteNew(); SUITE_ADD_TEST(suite, test_piracy_cmd_errors); + SUITE_ADD_TEST(suite, test_piracy_cmd); return suite; } From d252286f4ce840618ab299cc47526a9420d4b5c9 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Mon, 12 Oct 2015 18:54:47 +0200 Subject: [PATCH 05/10] refactor: extract function parse_ids for readability --- src/piracy.c | 58 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/src/piracy.c b/src/piracy.c index 93e4e346e..69b8b26b0 100644 --- a/src/piracy.c +++ b/src/piracy.c @@ -76,26 +76,10 @@ static bool validate_pirate(unit *u, order *ord) { return true; } -void piracy_cmd(unit * u, order *ord) -{ - region *r = u->region; - ship *sh = u->ship, *sh2; - direction_t dir, target_dir = NODIRECTION; - struct { - const faction *target; - int value; - } aff[MAXDIRECTIONS]; - int saff = 0; - int *il = NULL; +int *parse_ids(const order *ord) { const char *s; - attrib *a; + int *il = NULL; - if (!validate_pirate(u, ord)) { - return; - } - - /* Feststellen, ob schon ein anderer alliierter Pirat ein - * Ziel gefunden hat. */ init_order(ord); s = getstrtoken(); if (s != NULL && *s) { @@ -105,6 +89,12 @@ void piracy_cmd(unit * u, order *ord) s = getstrtoken(); } } + return il; +} + +direction_t find_piracy_target(unit *u, int *il) { + attrib *a; + region *r = u->region; for (a = a_find(r->attribs, &at_piracy_direction); a && a->type == &at_piracy_direction; a = a->next) { @@ -114,11 +104,34 @@ void piracy_cmd(unit * u, order *ord) if (alliedunit(u, p, HELP_FIGHT)) { if (il == 0 || (t && intlist_find(il, t->no))) { - target_dir = data->dir; - break; + return data->dir; } } } + return NODIRECTION; +} + +void piracy_cmd(unit * u, order *ord) +{ + region *r = u->region; + ship *sh = u->ship, *sh2; + direction_t dir, target_dir; + struct { + const faction *target; + int value; + } aff[MAXDIRECTIONS]; + int saff = 0; + int *il; + + if (!validate_pirate(u, ord)) { + return; + } + + il = parse_ids(ord); + /* Feststellen, ob schon ein anderer alliierter Pirat ein + * Ziel gefunden hat. */ + + target_dir = find_piracy_target(u, il); /* Wenn nicht, sehen wir, ob wir ein Ziel finden. */ @@ -138,7 +151,7 @@ void piracy_cmd(unit * u, order *ord) faction *f = visible_faction(cap->faction, cap); if (alliedunit(u, f, HELP_FIGHT)) continue; - if (il == 0 || intlist_find(il, cap->faction->no)) { + if (!il || intlist_find(il, cap->faction->no)) { // TODO: shouldn't this be f->no? ++aff[dir].value; if (rng_int() % aff[dir].value == 0) { aff[dir].target = f; @@ -160,8 +173,7 @@ void piracy_cmd(unit * u, order *ord) saff -= aff[dir].value; } target_dir = dir; - a = - a_add(&r->attribs, mk_piracy(u->faction, aff[dir].target, target_dir)); + a_add(&r->attribs, mk_piracy(u->faction, aff[dir].target, target_dir)); } } From 7e2364c296004cb644bd8e21a12094320d63467a Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Mon, 12 Oct 2015 19:40:20 +0200 Subject: [PATCH 06/10] test for actual piracy ship movement refactor storm check, run less code when disabled fix ship creation in tests --- src/move.c | 93 +++++++++++++++++++++++------------------------ src/piracy.c | 3 +- src/piracy.test.c | 28 +++++++++++--- src/tests.c | 3 +- 4 files changed, 73 insertions(+), 54 deletions(-) diff --git a/src/move.c b/src/move.c index c71f45c4f..8eaf052ca 100644 --- a/src/move.c +++ b/src/move.c @@ -1847,66 +1847,65 @@ sail(unit * u, order * ord, bool move_on_land, region_list ** routep) } if (!flying_ship(sh)) { - int stormchance; - int stormyness = 0; + int stormchance = 0; int reason; bool storms_enabled = get_param_int(global.parameters, "rules.ship.storms", 1) != 0; if (storms_enabled) { + int stormyness; gamedate date; get_gamedate(turn, &date); stormyness = storms ? storms[date.month] * 5 : 0; - } - /* storms should be the first thing we do. */ - stormchance = stormyness / shipspeed(sh, u); - if (check_leuchtturm(next_point, NULL)) { - int param = get_param_int(global.parameters, "rules.lighthous.stormchancedevisor", 0); - if (param > 0) { - stormchance /= param; + /* storms should be the first thing we do. */ + stormchance = stormyness / shipspeed(sh, u); + if (check_leuchtturm(next_point, NULL)) { + int param = get_param_int(global.parameters, "rules.lighthous.stormchancedevisor", 0); + if (param > 0) { + stormchance /= param; + } + else { + stormchance = 0; + } } - else { - stormchance = 0; - } - } - if (rng_int() % 10000 < stormchance * sh->type->storm - && fval(current_point->terrain, SEA_REGION)) { - if (!is_cursed(sh->attribs, C_SHIP_NODRIFT, 0)) { - region *rnext = NULL; - bool storm = true; - int d_offset = rng_int() % MAXDIRECTIONS; - direction_t d; - /* Sturm nur, wenn nächste Region Hochsee ist. */ - for (d = 0; d != MAXDIRECTIONS; ++d) { - direction_t dnext = (direction_t)((d + d_offset) % MAXDIRECTIONS); - region *rn = rconnect(current_point, dnext); + if (rng_int() % 10000 < stormchance * sh->type->storm + && fval(current_point->terrain, SEA_REGION)) { + if (!is_cursed(sh->attribs, C_SHIP_NODRIFT, 0)) { + region *rnext = NULL; + bool storm = true; + int d_offset = rng_int() % MAXDIRECTIONS; + direction_t d; + /* Sturm nur, wenn nächste Region Hochsee ist. */ + for (d = 0; d != MAXDIRECTIONS; ++d) { + direction_t dnext = (direction_t)((d + d_offset) % MAXDIRECTIONS); + region *rn = rconnect(current_point, dnext); - if (rn != NULL) { - if (fval(rn->terrain, FORBIDDEN_REGION)) - continue; - if (!fval(rn->terrain, SEA_REGION)) { - storm = false; - break; + if (rn != NULL) { + if (fval(rn->terrain, FORBIDDEN_REGION)) + continue; + if (!fval(rn->terrain, SEA_REGION)) { + storm = false; + break; + } + if (rn != next_point) + rnext = rn; } - if (rn != next_point) - rnext = rn; + } + if (storm && rnext != NULL) { + ADDMSG(&f->msgs, msg_message("storm", "ship region sink", + sh, current_point, sh->damage >= sh->size * DAMAGE_SCALE)); + + /* damage the ship. we handle destruction in the end */ + damage_ship(sh, damage_drift()); + if (sh->damage >= sh->size * DAMAGE_SCALE) + break; + + next_point = rnext; + /* these values need to be updated if next_point changes (due to storms): */ + tnext = next_point->terrain; } } - if (storm && rnext != NULL) { - ADDMSG(&f->msgs, msg_message("storm", "ship region sink", - sh, current_point, sh->damage >= sh->size * DAMAGE_SCALE)); - - /* damage the ship. we handle destruction in the end */ - damage_ship(sh, damage_drift()); - if (sh->damage >= sh->size * DAMAGE_SCALE) - break; - - next_point = rnext; - /* these values need to be updated if next_point changes (due to storms): */ - tnext = next_point->terrain; - } } - } - + } // storms_enabled if (!fval(tthis, SEA_REGION)) { if (!fval(tnext, SEA_REGION)) { if (!move_on_land) { diff --git a/src/piracy.c b/src/piracy.c index 69b8b26b0..ab68e4dae 100644 --- a/src/piracy.c +++ b/src/piracy.c @@ -115,7 +115,7 @@ void piracy_cmd(unit * u, order *ord) { region *r = u->region; ship *sh = u->ship, *sh2; - direction_t dir, target_dir; + direction_t target_dir; struct { const faction *target; int value; @@ -136,6 +136,7 @@ void piracy_cmd(unit * u, order *ord) /* Wenn nicht, sehen wir, ob wir ein Ziel finden. */ if (target_dir == NODIRECTION) { + direction_t dir; /* Einheit ist also Kapitän. Jetzt gucken, in wievielen * Nachbarregionen potentielle Opfer sind. */ diff --git a/src/piracy.test.c b/src/piracy.test.c index 250fc8b10..c521b1be6 100644 --- a/src/piracy.test.c +++ b/src/piracy.test.c @@ -1,6 +1,8 @@ #include #include "piracy.h" +#include +#include #include #include #include @@ -17,34 +19,48 @@ static void setup_piracy(void) { struct locale *lang; terrain_type *t_ocean; + ship_type *st_boat; test_cleanup(); + set_param(&global.parameters, "rules.ship.storms", "0"); lang = get_or_create_locale("de"); locale_setstring(lang, directions[D_EAST], "OSTEN"); init_directions(lang); t_ocean = test_create_terrain("ocean", SAIL_INTO | SEA_REGION); + st_boat = test_create_shiptype("boat"); + st_boat->cargo = 1000; } static void test_piracy_cmd(CuTest * tc) { faction *f; + region *r; unit *u, *u2; order *ord; terrain_type *t_ocean; + ship_type *st_boat; setup_piracy(); t_ocean = get_or_create_terrain("ocean"); + st_boat = st_get_or_create("boat"); u2 = test_create_unit(test_create_faction(0), test_create_region(1, 0, t_ocean)); - u_set_ship(u2, test_create_ship(u2->region, 0)); + u_set_ship(u2, test_create_ship(u2->region, st_boat)); assert(u2); - u = test_create_unit(f = test_create_faction(0), test_create_region(0, 0, t_ocean)); - u_set_ship(u, test_create_ship(u->region, 0)); + u = test_create_unit(f = test_create_faction(0), r = test_create_region(0, 0, t_ocean)); + set_level(u, SK_SAILING, st_boat->sumskill); + u_set_ship(u, test_create_ship(u->region, st_boat)); assert(f && u); f->locale = get_or_create_locale("de"); ord = create_order(K_PIRACY, f->locale, "%s", itoa36(u2->faction->no)); assert(ord); piracy_cmd(u, ord); - CuAssertPtrNotNullMsg(tc, "successful PIRACY", test_find_messagetype(f->msgs, "piratesawvictim")); + CuAssertPtrEquals(tc, 0, u->thisorder); + CuAssertTrue(tc, u->region != r); + CuAssertPtrEquals(tc, u2->region, u->region); + CuAssertPtrEquals(tc, u2->region, u->ship->region); + CuAssertPtrNotNullMsg(tc, "successful PIRACY sets attribute", r->attribs); // FIXME: this is testing implementation, not interface + CuAssertPtrNotNullMsg(tc, "successful PIRACY message", test_find_messagetype(f->msgs, "piratesawvictim")); + CuAssertPtrNotNullMsg(tc, "successful PIRACY movement", test_find_messagetype(f->msgs, "shipsail")); test_cleanup(); } @@ -53,8 +69,10 @@ static void test_piracy_cmd_errors(CuTest * tc) { faction *f; unit *u, *u2; order *ord; + ship_type *st_boat; setup_piracy(); + st_boat = st_get_or_create("boat"); u = test_create_unit(f = test_create_faction(0), test_create_region(0, 0, get_or_create_terrain("ocean"))); f->locale = get_or_create_locale("de"); ord = create_order(K_PIRACY, f->locale, ""); @@ -63,7 +81,7 @@ static void test_piracy_cmd_errors(CuTest * tc) { piracy_cmd(u, ord); CuAssertPtrNotNullMsg(tc, "must be on a ship for PIRACY", test_find_messagetype(f->msgs, "error144")); - u_set_ship(u, test_create_ship(u->region, 0)); + u_set_ship(u, test_create_ship(u->region, st_boat)); u2 = test_create_unit(u->faction, u->region); u_set_ship(u2, u->ship); diff --git a/src/tests.c b/src/tests.c index b8c5f06c1..dce6604ce 100644 --- a/src/tests.c +++ b/src/tests.c @@ -123,7 +123,7 @@ building * test_create_building(region * r, const building_type * btype) ship * test_create_ship(region * r, const ship_type * stype) { - ship * s = new_ship(stype ? stype : st_get_or_create("boat"), r, default_locale); + ship * s = new_ship(stype ? stype : test_create_shiptype("boat"), r, default_locale); s->size = s->type->construction ? s->type->construction->maxsize : 1; return s; } @@ -134,6 +134,7 @@ ship_type * test_create_shiptype(const char * name) stype->cptskill = 1; stype->sumskill = 1; stype->minskill = 1; + stype->range = 2; if (!stype->construction) { stype->construction = calloc(1, sizeof(construction)); stype->construction->maxsize = 5; From 177f8f52881007a5ed6696880537767fdebc6a46 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Tue, 13 Oct 2015 13:32:27 +0200 Subject: [PATCH 07/10] re-enable a test that supposedly breaks on travis (find out why) fix a potential null-pointer crash in rename_cmd --- src/laws.c | 2 +- src/laws.test.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/laws.c b/src/laws.c index ba5466fda..549acb2b2 100755 --- a/src/laws.c +++ b/src/laws.c @@ -1622,7 +1622,7 @@ bool renamed_building(const building * b) static int rename_cmd(unit * u, order * ord, char **s, const char *s2) { - if (!s2[0]) { + if (!s2 || !s2[0]) { cmistake(u, ord, 84, MSG_EVENT); return 0; } diff --git a/src/laws.test.c b/src/laws.test.c index c7c3bf632..7b733b9cf 100644 --- a/src/laws.test.c +++ b/src/laws.test.c @@ -1135,7 +1135,7 @@ CuSuite *get_laws_suite(void) SUITE_ADD_TEST(suite, test_mail_faction_no_msg); SUITE_ADD_TEST(suite, test_mail_region_no_msg); SUITE_ADD_TEST(suite, test_mail_faction_no_target); - (void)test_luck_message; /* disabled, breaks on travis */ + SUITE_ADD_TEST(suite, test_luck_message); return suite; } From 6de604701daddbfd72bc1d2a4581fb1541e3f930 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Tue, 13 Oct 2015 13:56:58 +0200 Subject: [PATCH 08/10] add some test coverage for name_cmd (not covering all error cases yet) --- src/laws.test.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++ src/tests.c | 5 ++- 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/src/laws.test.c b/src/laws.test.c index 7b733b9cf..8c4a7d341 100644 --- a/src/laws.test.c +++ b/src/laws.test.c @@ -765,6 +765,83 @@ static void test_luck_message(CuTest *tc) { test_cleanup(); } +static unit * setup_name_cmd(void) { + faction *f; + struct locale *lang; + + test_cleanup(); + f = test_create_faction(0); + f->locale = lang = get_or_create_locale("en"); + locale_setstring(lang, parameters[P_UNIT], "UNIT"); + locale_setstring(lang, parameters[P_REGION], "REGION"); + locale_setstring(lang, parameters[P_FACTION], "FACTION"); + locale_setstring(lang, parameters[P_BUILDING], "BUILDING"); + locale_setstring(lang, parameters[P_SHIP], "SHIP"); + init_parameters(lang); + return test_create_unit(f, test_create_region(0, 0, 0)); +} + +static void test_name_unit(CuTest *tc) { + unit *u; + order *ord; + + u = setup_name_cmd(); + ord = create_order(K_NAME, u->faction->locale, "UNIT Hodor"); + name_cmd(u, ord); + CuAssertStrEquals(tc, "Hodor", u->_name); + test_cleanup(); +} + +static void test_name_region(CuTest *tc) { + unit *u; + order *ord; + + u = setup_name_cmd(); + ord = create_order(K_NAME, u->faction->locale, "REGION Hodor"); + + name_cmd(u, ord); + CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error145")); + + u->building = test_create_building(u->region, 0); + name_cmd(u, ord); + CuAssertStrEquals(tc, "Hodor", u->region->land->name); + + test_cleanup(); +} + +static void test_name_building(CuTest *tc) { + unit *u; + order *ord; + + u = setup_name_cmd(); + ord = create_order(K_NAME, u->faction->locale, "BUILDING Hodor"); + + name_cmd(u, ord); + CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error145")); + + u->building = test_create_building(u->region, 0); + name_cmd(u, ord); + CuAssertStrEquals(tc, "Hodor", u->building->name); +/* TODO: test BTF_NAMECHANGE: + btype->flags |= BTF_NAMECHANGE; + CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error278")); + test_clear_messages(u->faction); + name_cmd(u, ord); */ + test_cleanup(); +} + +static void test_name_ship(CuTest *tc) { + unit *u; + order *ord; + + u = setup_name_cmd(); + u->ship = test_create_ship(u->region, 0); + ord = create_order(K_NAME, u->faction->locale, "SHIP Hodor"); + name_cmd(u, ord); + CuAssertStrEquals(tc, "Hodor", u->ship->name); + test_cleanup(); +} + static void test_long_order_normal(CuTest *tc) { // TODO: write more tests unit *u; @@ -1136,6 +1213,10 @@ CuSuite *get_laws_suite(void) SUITE_ADD_TEST(suite, test_mail_region_no_msg); SUITE_ADD_TEST(suite, test_mail_faction_no_target); SUITE_ADD_TEST(suite, test_luck_message); + SUITE_ADD_TEST(suite, test_name_unit); + SUITE_ADD_TEST(suite, test_name_region); + SUITE_ADD_TEST(suite, test_name_building); + SUITE_ADD_TEST(suite, test_name_ship); return suite; } diff --git a/src/tests.c b/src/tests.c index dce6604ce..741812590 100644 --- a/src/tests.c +++ b/src/tests.c @@ -116,7 +116,7 @@ test_create_terrain(const char * name, unsigned int flags) building * test_create_building(region * r, const building_type * btype) { - building * b = new_building(btype ? btype : bt_get_or_create("castle"), r, default_locale); + building * b = new_building(btype ? btype : test_create_buildingtype("castle"), r, default_locale); b->size = b->type->maxsize > 0 ? b->type->maxsize : 1; return b; } @@ -150,7 +150,7 @@ ship_type * test_create_shiptype(const char * name) building_type * test_create_buildingtype(const char * name) { - building_type *btype = (building_type *)calloc(sizeof(building_type), 1); + building_type *btype = bt_get_or_create(name); btype->flags = BTF_NAMECHANGE; btype->_name = _strdup(name); btype->construction = (construction *)calloc(sizeof(construction), 1); @@ -165,7 +165,6 @@ building_type * test_create_buildingtype(const char * name) if (default_locale) { locale_setstring(default_locale, name, name); } - bt_register(btype); return btype; } From 96be12218e1cf3be1935d8988af42e08fd874360 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Tue, 13 Oct 2015 14:27:42 +0200 Subject: [PATCH 09/10] test rename with missing name (not crashing, error message) --- src/laws.c | 46 ++++++++++++++++++++++++++++------------------ src/laws.test.c | 39 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 64 insertions(+), 21 deletions(-) diff --git a/src/laws.c b/src/laws.c index 549acb2b2..2fd9ce1d8 100755 --- a/src/laws.c +++ b/src/laws.c @@ -1622,7 +1622,8 @@ bool renamed_building(const building * b) static int rename_cmd(unit * u, order * ord, char **s, const char *s2) { - if (!s2 || !s2[0]) { + assert(s2); + if (!s2[0]) { cmistake(u, ord, 84, MSG_EVENT); return 0; } @@ -1638,48 +1639,53 @@ static int rename_cmd(unit * u, order * ord, char **s, const char *s2) return 0; } -int -rename_building(unit * u, order * ord, building * b, const char *name) -{ +static bool try_rename(unit *u, building *b, order *ord) { unit *owner = b ? building_owner(b) : 0; bool foreign = !(owner && owner->faction == u->faction); if (!b) { cmistake(u, ord, u->building ? 6 : 145, MSG_EVENT); - return -1; + return false; } if (!fval(b->type, BTF_NAMECHANGE) && renamed_building(b)) { cmistake(u, ord, 278, MSG_EVENT); - return -1; + return false; } if (foreign) { if (renamed_building(b)) { cmistake(u, ord, 246, MSG_EVENT); - return -1; + return false; } if (owner) { if (cansee(owner->faction, u->region, u, 0)) { ADDMSG(&owner->faction->msgs, msg_message("renamed_building_seen", - "building renamer region", b, u, u->region)); + "building renamer region", b, u, u->region)); } else { ADDMSG(&owner->faction->msgs, msg_message("renamed_building_notseen", - "building region", b, u->region)); + "building region", b, u->region)); + } + if (owner != u) { + cmistake(u, ord, 148, MSG_PRODUCE); + return false; } } } - else { - if (owner != u) { - cmistake(u, ord, 148, MSG_PRODUCE); - return -1; - } - } + return true; +} +int +rename_building(unit * u, order * ord, building * b, const char *name) +{ + assert(name); + if (!try_rename(u, b, ord)) { + return -1; + } return rename_cmd(u, ord, &b->name, name); } @@ -1718,9 +1724,10 @@ int name_cmd(struct unit *u, struct order *ord) if (foreign) { b = getbuilding(u->region); } - - return rename_building(u, ord, b, getstrtoken()); - + if (try_rename(u, b, ord)) { + s = &b->name; + } + break; case P_FACTION: if (foreign) { faction *f; @@ -1896,6 +1903,9 @@ int name_cmd(struct unit *u, struct order *ord) if (name) { rename_cmd(u, ord, s, name); } + else { + cmistake(u, ord, 84, MSG_EVENT); + } } return 0; diff --git a/src/laws.test.c b/src/laws.test.c index 8c4a7d341..5918001d9 100644 --- a/src/laws.test.c +++ b/src/laws.test.c @@ -786,9 +786,18 @@ static void test_name_unit(CuTest *tc) { order *ord; u = setup_name_cmd(); + ord = create_order(K_NAME, u->faction->locale, "UNIT Hodor"); name_cmd(u, ord); CuAssertStrEquals(tc, "Hodor", u->_name); + free_order(ord); + + ord = create_order(K_NAME, u->faction->locale, "UNIT"); + name_cmd(u, ord); + CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error84")); + CuAssertStrEquals(tc, "Hodor", u->_name); + free_order(ord); + test_cleanup(); } @@ -797,14 +806,21 @@ static void test_name_region(CuTest *tc) { order *ord; u = setup_name_cmd(); - ord = create_order(K_NAME, u->faction->locale, "REGION Hodor"); + ord = create_order(K_NAME, u->faction->locale, "REGION Hodor"); name_cmd(u, ord); CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error145")); u->building = test_create_building(u->region, 0); name_cmd(u, ord); CuAssertStrEquals(tc, "Hodor", u->region->land->name); + free_order(ord); + + ord = create_order(K_NAME, u->faction->locale, "REGION"); + name_cmd(u, ord); + CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error84")); + CuAssertStrEquals(tc, "Hodor", u->region->land->name); + free_order(ord); test_cleanup(); } @@ -814,15 +830,23 @@ static void test_name_building(CuTest *tc) { order *ord; u = setup_name_cmd(); - ord = create_order(K_NAME, u->faction->locale, "BUILDING Hodor"); + ord = create_order(K_NAME, u->faction->locale, "BUILDING Hodor"); name_cmd(u, ord); CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error145")); u->building = test_create_building(u->region, 0); name_cmd(u, ord); CuAssertStrEquals(tc, "Hodor", u->building->name); -/* TODO: test BTF_NAMECHANGE: + free_order(ord); + + ord = create_order(K_NAME, u->faction->locale, "BUILDING"); + name_cmd(u, ord); + CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error84")); + CuAssertStrEquals(tc, "Hodor", u->building->name); + free_order(ord); + + /* TODO: test BTF_NAMECHANGE: btype->flags |= BTF_NAMECHANGE; CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error278")); test_clear_messages(u->faction); @@ -836,9 +860,18 @@ static void test_name_ship(CuTest *tc) { u = setup_name_cmd(); u->ship = test_create_ship(u->region, 0); + ord = create_order(K_NAME, u->faction->locale, "SHIP Hodor"); name_cmd(u, ord); CuAssertStrEquals(tc, "Hodor", u->ship->name); + free_order(ord); + + ord = create_order(K_NAME, u->faction->locale, "SHIP"); + name_cmd(u, ord); + CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error84")); + CuAssertStrEquals(tc, "Hodor", u->ship->name); + free_order(ord); + test_cleanup(); } From 7854684d8b1f3fa008e94ce77deb581fdcf508f5 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Tue, 13 Oct 2015 15:45:38 +0200 Subject: [PATCH 10/10] fix memory leaks in all tests that create an order without calling free_order at the end. --- src/economy.test.c | 8 ++------ src/give.test.c | 3 +++ src/kernel/jsonconf.test.c | 1 + src/kernel/order.test.c | 1 + src/keyword.test.c | 1 + src/laws.test.c | 22 ++++++++++++++++------ src/piracy.test.c | 2 ++ src/spy.test.c | 3 +++ 8 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/economy.test.c b/src/economy.test.c index fe31e5976..08b8bea56 100644 --- a/src/economy.test.c +++ b/src/economy.test.c @@ -141,14 +141,12 @@ static struct unit *create_recruiter(void) { static void test_heroes_dont_recruit(CuTest * tc) { unit *u; - order *ord; test_cleanup(); u = create_recruiter(); fset(u, UFL_HERO); - ord = create_order(K_RECRUIT, default_locale, "1"); - unit_addorder(u, ord); + unit_addorder(u, create_order(K_RECRUIT, default_locale, "1")); economics(u->region); @@ -160,13 +158,11 @@ static void test_heroes_dont_recruit(CuTest * tc) { static void test_normals_recruit(CuTest * tc) { unit *u; - order *ord; test_cleanup(); u = create_recruiter(); - ord = create_order(K_RECRUIT, default_locale, "1"); - unit_addorder(u, ord); + unit_addorder(u, create_order(K_RECRUIT, default_locale, "1")); economics(u->region); diff --git a/src/give.test.c b/src/give.test.c index c9e005b8f..401cd1fb8 100644 --- a/src/give.test.c +++ b/src/give.test.c @@ -217,6 +217,7 @@ static void test_give_men_requires_contact(CuTest * tc) { give_cmd(env.src, ord); CuAssertPtrEquals(tc, 0, test_find_messagetype(env.f1->msgs, "give_person")); CuAssertPtrNotNull(tc, test_find_messagetype(env.f1->msgs, "feedback_no_contact")); + free_order(ord); test_cleanup(); } @@ -290,6 +291,7 @@ static void test_give_herbs(CuTest * tc) { give_cmd(env.src, ord); CuAssertIntEquals(tc, 0, i_get(env.src->items, env.itype)); CuAssertIntEquals(tc, 10, i_get(env.dst->items, env.itype)); + free_order(ord); test_cleanup(); } @@ -342,6 +344,7 @@ static void test_give_invalid_target(CuTest *tc) { give_cmd(env.src, ord); CuAssertIntEquals(tc, 10, i_get(env.src->items, env.itype)); CuAssertPtrNotNull(tc, test_find_messagetype(env.f1->msgs, "feedback_unit_not_found")); + free_order(ord); test_cleanup(); } diff --git a/src/kernel/jsonconf.test.c b/src/kernel/jsonconf.test.c index d7a4ad0cd..2d9394749 100644 --- a/src/kernel/jsonconf.test.c +++ b/src/kernel/jsonconf.test.c @@ -577,6 +577,7 @@ static void test_infinitive_from_config(CuTest *tc) { ord = create_order(K_STUDY, lang, ""); CuAssertStrEquals(tc, "LERNE", get_command(ord, buffer, sizeof(buffer))); + free_order(ord); test_cleanup(); } diff --git a/src/kernel/order.test.c b/src/kernel/order.test.c index 6900da420..84dc61323 100644 --- a/src/kernel/order.test.c +++ b/src/kernel/order.test.c @@ -123,6 +123,7 @@ static void test_init_order(CuTest *tc) { CuAssertIntEquals(tc, K_MAKETEMP, init_order(ord)); CuAssertStrEquals(tc, "hurr", getstrtoken()); CuAssertStrEquals(tc, "durr", getstrtoken()); + free_order(ord); } static void test_getstrtoken(CuTest *tc) { diff --git a/src/keyword.test.c b/src/keyword.test.c index e0274d9cf..da87c860a 100644 --- a/src/keyword.test.c +++ b/src/keyword.test.c @@ -35,6 +35,7 @@ static void test_infinitive(CuTest *tc) { ord = create_order(K_STUDY, lang, ""); CuAssertStrEquals(tc, "LERNE", get_command(ord, buffer, sizeof(buffer))); + free_order(ord); test_cleanup(); } diff --git a/src/laws.test.c b/src/laws.test.c index 5918001d9..474f95276 100644 --- a/src/laws.test.c +++ b/src/laws.test.c @@ -467,6 +467,7 @@ static void test_reserve_cmd(CuTest *tc) { CuAssertIntEquals(tc, 200, reserve_cmd(u1, ord)); CuAssertIntEquals(tc, 200, i_get(u1->items, rtype->itype)); CuAssertIntEquals(tc, 0, i_get(u2->items, rtype->itype)); + free_order(ord); test_cleanup(); } @@ -517,6 +518,7 @@ static void test_pay_cmd(CuTest *tc) { assert(ord); CuAssertIntEquals(tc, 0, pay_cmd(fix.u1, ord)); CuAssertIntEquals(tc, BLD_DONTPAY, b->flags&BLD_DONTPAY); + free_order(ord); test_cleanup(); } @@ -541,6 +543,7 @@ static void test_pay_cmd_other_building(CuTest *tc) { CuAssertPtrEquals(tc, fix.u1, building_owner(b)); CuAssertIntEquals(tc, 0, pay_cmd(fix.u1, ord)); CuAssertIntEquals(tc, BLD_DONTPAY, b->flags&BLD_DONTPAY); + free_order(ord); test_cleanup(); } @@ -559,6 +562,7 @@ static void test_pay_cmd_must_be_owner(CuTest *tc) { assert(ord); CuAssertIntEquals(tc, 0, pay_cmd(fix.u2, ord)); CuAssertIntEquals(tc, 0, b->flags&BLD_DONTPAY); + free_order(ord); test_cleanup(); } @@ -566,8 +570,8 @@ static void test_new_units(CuTest *tc) { unit *u; faction *f; region *r; - order *ord; const struct locale *loc; + test_cleanup(); test_create_world(); f = test_create_faction(NULL); @@ -577,9 +581,7 @@ static void test_new_units(CuTest *tc) { assert(u && !u->next); loc = get_locale("de"); assert(loc); - ord = create_order(K_MAKETEMP, loc, "hurr"); - assert(ord); - u->orders = ord; + u->orders = create_order(K_MAKETEMP, loc, "hurr"); new_units(); CuAssertPtrNotNull(tc, u->next); test_cleanup(); @@ -708,6 +710,7 @@ static void test_reserve_self(CuTest *tc) { CuAssertIntEquals(tc, 100, reserve_self(u1, ord)); CuAssertIntEquals(tc, 100, i_get(u1->items, rtype->itype)); CuAssertIntEquals(tc, 100, i_get(u2->items, rtype->itype)); + free_order(ord); test_cleanup(); } @@ -879,13 +882,13 @@ static void test_long_order_normal(CuTest *tc) { // TODO: write more tests unit *u; order *ord; + test_cleanup(); u = test_create_unit(test_create_faction(0), test_create_region(0, 0, 0)); fset(u, UFL_MOVED); fset(u, UFL_LONGACTION); u->faction->locale = get_or_create_locale("de"); - ord = create_order(K_MOVE, u->faction->locale, 0); - unit_addorder(u, ord); + unit_addorder(u, ord = create_order(K_MOVE, u->faction->locale, 0)); update_long_order(u); CuAssertPtrEquals(tc, ord->data, u->thisorder->data); CuAssertIntEquals(tc, 0, fval(u, UFL_MOVED)); @@ -1121,6 +1124,7 @@ static void test_mail_unit(CuTest *tc) { ord = create_order(K_MAIL, u->faction->locale, "EINHEIT %s 'Hodor!'", itoa36(u->no)); mail_cmd(u, ord); CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "unitmessage")); + free_order(ord); test_cleanup(); } @@ -1132,6 +1136,7 @@ static void test_mail_faction(CuTest *tc) { ord = create_order(K_MAIL, u->faction->locale, "PARTEI %s 'Hodor!'", itoa36(u->faction->no)); mail_cmd(u, ord); CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "regionmessage")); + free_order(ord); test_cleanup(); } @@ -1143,6 +1148,7 @@ static void test_mail_region(CuTest *tc) { ord = create_order(K_MAIL, u->faction->locale, "REGION 'Hodor!'", itoa36(u->no)); mail_cmd(u, ord); CuAssertPtrNotNull(tc, test_find_messagetype(u->region->msgs, "mail_result")); + free_order(ord); test_cleanup(); } @@ -1155,6 +1161,7 @@ static void test_mail_unit_no_msg(CuTest *tc) { mail_cmd(u, ord); CuAssertPtrEquals(tc, 0, test_find_messagetype(u->faction->msgs, "unitmessage")); CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error30")); + free_order(ord); test_cleanup(); } @@ -1167,6 +1174,7 @@ static void test_mail_faction_no_msg(CuTest *tc) { mail_cmd(u, ord); CuAssertPtrEquals(tc, 0, test_find_messagetype(u->faction->msgs, "regionmessage")); CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error30")); + free_order(ord); test_cleanup(); } @@ -1179,6 +1187,7 @@ static void test_mail_faction_no_target(CuTest *tc) { mail_cmd(u, ord); CuAssertPtrEquals(tc, 0, test_find_messagetype(u->faction->msgs, "regionmessage")); CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error66")); + free_order(ord); test_cleanup(); } @@ -1191,6 +1200,7 @@ static void test_mail_region_no_msg(CuTest *tc) { mail_cmd(u, ord); CuAssertPtrEquals(tc, 0, test_find_messagetype(u->region->msgs, "mail_result")); CuAssertPtrNotNull(tc, test_find_messagetype(u->faction->msgs, "error30")); + free_order(ord); test_cleanup(); } diff --git a/src/piracy.test.c b/src/piracy.test.c index c521b1be6..9074c1e33 100644 --- a/src/piracy.test.c +++ b/src/piracy.test.c @@ -61,6 +61,7 @@ static void test_piracy_cmd(CuTest * tc) { CuAssertPtrNotNullMsg(tc, "successful PIRACY sets attribute", r->attribs); // FIXME: this is testing implementation, not interface CuAssertPtrNotNullMsg(tc, "successful PIRACY message", test_find_messagetype(f->msgs, "piratesawvictim")); CuAssertPtrNotNullMsg(tc, "successful PIRACY movement", test_find_messagetype(f->msgs, "shipsail")); + free_order(ord); test_cleanup(); } @@ -93,6 +94,7 @@ static void test_piracy_cmd_errors(CuTest * tc) { test_clear_messages(f); piracy_cmd(u, ord); CuAssertPtrNotNullMsg(tc, "must specify target for PIRACY", test_find_messagetype(f->msgs, "piratenovictim")); + free_order(ord); test_cleanup(); } diff --git a/src/spy.test.c b/src/spy.test.c index 95f1a6dbc..00a1767d0 100644 --- a/src/spy.test.c +++ b/src/spy.test.c @@ -110,6 +110,7 @@ static void test_sabotage_self(CuTest *tc) { assert(ord); CuAssertIntEquals(tc, 0, sabotage_cmd(u, ord)); CuAssertPtrEquals(tc, 0, r->ships); + free_order(ord); test_cleanup(); } @@ -139,6 +140,7 @@ static void test_sabotage_other_fail(CuTest *tc) { msg = test_get_last_message(u->faction->msgs); CuAssertStrEquals(tc, "destroy_ship_3", test_get_messagetype(msg)); CuAssertPtrNotNull(tc, r->ships); + free_order(ord); test_cleanup(); } @@ -164,6 +166,7 @@ static void test_sabotage_other_success(CuTest *tc) { set_level(u2, SK_SPY, 1); CuAssertIntEquals(tc, 0, sabotage_cmd(u2, ord)); CuAssertPtrEquals(tc, 0, r->ships); + free_order(ord); test_cleanup(); }