Author: sveinung Date: Sat Oct 24 23:33:15 2015 New Revision: 30201 URL: http://svn.gna.org/viewcvs/freeciv?rev=30201&view=rev Log: Make target/actor exists sanity checks asserts
The checks that actor and target survived the Lua in the action doer functions are redundant because of the checks in unit_perform_action(). Change them into asserts. See patch #6467 Modified: trunk/server/diplomats.c trunk/server/unithand.c Modified: trunk/server/diplomats.c URL: http://svn.gna.org/viewcvs/freeciv/trunk/server/diplomats.c?rev=30201&r1=30200&r2=30201&view=diff ============================================================================== --- trunk/server/diplomats.c (original) +++ trunk/server/diplomats.c Sat Oct 24 23:33:15 2015 @@ -95,19 +95,13 @@ const char *clink; /* Fetch target city's player. Sanity checks. */ - if (!pcity) { - return FALSE; - } - + fc_assert_ret_val(pcity, FALSE); cplayer = city_owner(pcity); - if (!cplayer) { - return FALSE; - } + fc_assert_ret_val(cplayer, FALSE); /* Sanity check: The actor still exists. */ - if (!pplayer || !pdiplomat) { - return FALSE; - } + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(pdiplomat, FALSE); ctile = city_tile(pcity); clink = city_link(pcity); @@ -179,16 +173,17 @@ struct traderoute_packet_list *routes; /* Fetch target city's player. Sanity checks. */ - if (!pcity) { - return FALSE; - } - - cplayer = city_owner (pcity); - if ((cplayer == pplayer) || !cplayer) - return FALSE; + fc_assert_ret_val(pcity, FALSE); + cplayer = city_owner(pcity); + fc_assert_ret_val(cplayer, FALSE); /* Sanity check: The actor still exists. */ - if (!pplayer || !pdiplomat) { + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(pdiplomat, FALSE); + + /* Sanity check: The target is foreign. */ + if (cplayer == pplayer) { + /* Nothing to do to a domestic target. */ return FALSE; } @@ -293,17 +288,17 @@ struct player *cplayer; /* Fetch target city's player. Sanity checks. */ - if (!pcity) { - return FALSE; - } - + fc_assert_ret_val(pcity, FALSE); cplayer = city_owner(pcity); - if ((cplayer == pplayer) || !cplayer) { - return FALSE; - } + fc_assert_ret_val(cplayer, FALSE); /* Sanity check: The actor still exists. */ - if (!pplayer || !pdiplomat) { + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(pdiplomat, FALSE); + + /* Sanity check: The target is foreign. */ + if (cplayer == pplayer) { + /* Nothing to do to a domestic target. */ return FALSE; } @@ -364,19 +359,13 @@ struct player *uplayer; /* Fetch target unit's player. Sanity checks. */ - if (!pvictim) { - return FALSE; - } - + fc_assert_ret_val(pvictim, FALSE); uplayer = unit_owner(pvictim); - if (!uplayer) { - return FALSE; - } + fc_assert_ret_val(uplayer, FALSE); /* Sanity check: The actor still exists. */ - if (!pplayer || !pdiplomat) { - return FALSE; - } + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(pdiplomat, FALSE); log_debug("sabotage-unit: unit: %d", pdiplomat->id); @@ -466,18 +455,13 @@ struct city *pcity; /* Fetch target unit's player. Sanity checks. */ - if (!pvictim) { - return FALSE; - } + fc_assert_ret_val(pvictim, FALSE); uplayer = unit_owner(pvictim); - if (!uplayer) { - return FALSE; - } + fc_assert_ret_val(uplayer, FALSE); /* Sanity check: The actor still exists. */ - if (!pplayer || !pdiplomat) { - return FALSE; - } + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(pdiplomat, FALSE); /* Sanity check: The target is foreign. */ if (uplayer == pplayer) { @@ -619,17 +603,17 @@ /* We have to check arguments. They are sent to us by a client, so we cannot trust them */ - if (!pcity) { - return FALSE; - } - - cplayer = city_owner (pcity); - if ((cplayer == pplayer) || !cplayer) { - return FALSE; - } + fc_assert_ret_val(pcity, FALSE); + cplayer = city_owner(pcity); + fc_assert_ret_val(cplayer, FALSE); /* Sanity check: The actor still exists. */ - if (!pplayer || !pdiplomat) { + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(pdiplomat, FALSE); + + /* Sanity check: The target is foreign. */ + if (cplayer == pplayer) { + /* Nothing to do to a domestic target. */ return FALSE; } @@ -789,19 +773,13 @@ int revolt_cost; /* Fetch target civilization's player. Sanity checks. */ - if (!pcity) { - return FALSE; - } - - cplayer = city_owner (pcity); - if (!cplayer) { - return FALSE; - } + fc_assert_ret_val(pcity, FALSE); + cplayer = city_owner(pcity); + fc_assert_ret_val(cplayer, FALSE); /* Sanity check: The actor still exists. */ - if (!pplayer || !pdiplomat) { - return FALSE; - } + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(pdiplomat, FALSE); /* Sanity check: The target is foreign. */ if (cplayer == pplayer) { @@ -934,19 +912,13 @@ int count, which; /* Fetch target city's player. Sanity checks. */ - if (!pcity) { - return FALSE; - } - - cplayer = city_owner (pcity); - if (!cplayer) { - return FALSE; - } + fc_assert_ret_val(pcity, FALSE); + cplayer = city_owner(pcity); + fc_assert_ret_val(cplayer, FALSE); /* Sanity check: The actor still exists. */ - if (!pplayer || !pdiplomat) { - return FALSE; - } + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(pdiplomat, FALSE); log_debug("sabotage: unit: %d", pdiplomat->id); @@ -1207,22 +1179,17 @@ int gold_give; /* Sanity check: The actor still exists. */ - if (!act_player || !act_unit) { - return FALSE; - } + fc_assert_ret_val(act_player, FALSE); + fc_assert_ret_val(act_unit, FALSE); /* Sanity check: The target city still exists. */ - if (!tgt_city) { - return FALSE; - } + fc_assert_ret_val(tgt_city, FALSE); /* Who to steal from. */ tgt_player = city_owner(tgt_city); /* Sanity check: The target player still exists. */ - if (!tgt_player) { - return FALSE; - } + fc_assert_ret_val(tgt_player, FALSE); /* Sanity check: The target is foreign. */ if (tgt_player == act_player) { @@ -1343,22 +1310,17 @@ const char *tgt_city_link; /* Sanity check: The actor still exists. */ - if (!act_player || !act_unit) { - return FALSE; - } + fc_assert_ret_val(act_player, FALSE); + fc_assert_ret_val(act_unit, FALSE); /* Sanity check: The target city still exists. */ - if (!tgt_city) { - return FALSE; - } + fc_assert_ret_val(tgt_city, FALSE); /* Who to steal from. */ tgt_player = city_owner(tgt_city); /* Sanity check: The target player still exists. */ - if (!tgt_player) { - return FALSE; - } + fc_assert_ret_val(tgt_player, FALSE); /* Sanity check: The target is foreign. */ if (tgt_player == act_player) { @@ -1452,22 +1414,17 @@ const char *tgt_city_link; /* Sanity check: The actor still exists. */ - if (!act_player || !act_unit) { - return FALSE; - } + fc_assert_ret_val(act_player, FALSE); + fc_assert_ret_val(act_unit, FALSE); /* Sanity check: The target city still exists. */ - if (!tgt_city) { - return FALSE; - } + fc_assert_ret_val(tgt_city, FALSE); /* The victim. */ tgt_player = city_owner(tgt_city); /* Sanity check: The target player still exists. */ - if (!tgt_player) { - return FALSE; - } + fc_assert_ret_val(tgt_player, FALSE); tgt_tile = city_tile(tgt_city); tgt_city_link = city_link(tgt_city); Modified: trunk/server/unithand.c URL: http://svn.gna.org/viewcvs/freeciv/trunk/server/unithand.c?rev=30201&r1=30200&r2=30201&view=diff ============================================================================== --- trunk/server/unithand.c (original) +++ trunk/server/unithand.c Sat Oct 24 23:33:15 2015 @@ -286,6 +286,10 @@ char capturer_link[MAX_LEN_LINK]; const char *capturer_nation = nation_plural_for_player(pplayer); bv_unit_types unique_on_tile; + + /* Sanity check: The actor still exists. */ + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(punit, FALSE); /* Sanity check: make sure that the capture won't result in the actor * ending up with more than one unit of each unique unit type. */ @@ -2019,10 +2023,9 @@ { struct action *blocker; - if (!punit) { - /* The actor is dead. */ - return FALSE; - } + /* Sanity check: The actor still exists. */ + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(punit, FALSE); if (unit_has_type_flag(punit, UTYF_UNDISBANDABLE)) { /* refuse to kill ourselves */ @@ -2062,15 +2065,12 @@ struct unit *punit, struct city *pcity) { - /* Sanity check: The actor is still alive. */ - if (!punit) { - return FALSE; - } - - if (!pcity) { - /* City was destroyed during pre action Lua. */ - return FALSE; - } + /* Sanity check: The actor still exists. */ + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(punit, FALSE); + + /* Sanity check: The target city still exists. */ + fc_assert_ret_val(pcity, FALSE); /* Add the shields from recycling the unit to the city's current * production. */ @@ -2171,14 +2171,10 @@ struct city *pcity) { /* Sanity check: The actor is still alive. */ - if (!punit) { - return FALSE; - } - - if (!pcity) { - /* City was destroyed during pre action Lua. */ - return FALSE; - } + fc_assert_ret_val(punit, FALSE); + + /* Sanity check: The target city still exists. */ + fc_assert_ret_val(pcity, FALSE); fc_assert_ret_val(unit_pop_value(punit) > 0, FALSE); city_size_add(pcity, unit_pop_value(punit)); @@ -2231,6 +2227,10 @@ int size; struct player *nationality; struct player *towner; + + /* Sanity check: The actor still exists. */ + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(punit, FALSE); towner = tile_owner(ptile); @@ -2455,10 +2455,9 @@ struct player *pplayer = unit_owner(punit); struct city *pcity = tile_city(ptile); - if (!punit) { - /* Actor unit was destroyed during pre action Lua. */ - return FALSE; - } + /* Sanity check: The actor still exists. */ + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(punit, FALSE); log_debug("Start bombard: %s %s to %d, %d.", nation_rule_name(nation_of_player(pplayer)), @@ -2550,10 +2549,9 @@ { struct city *pcity; - if (!punit) { - /* Actor unit was destroyed during pre action Lua. */ - return FALSE; - } + /* Sanity check: The actor still exists. */ + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(punit, FALSE); log_debug("Start nuclear attack: %s %s against (%d, %d).", nation_rule_name(nation_of_player(pplayer)), @@ -2614,30 +2612,17 @@ struct player *tgt_player; bool try_civil_war = FALSE; - if (!act_player) { - /* Someone should be performing the action. */ - fc_assert(act_player); - - return FALSE; - } - - if (!tgt_city) { - /* City was destroyed during pre action Lua. */ - return FALSE; - } + /* Sanity check: The actor still exists. */ + fc_assert_ret_val(act_player, FALSE); + fc_assert_ret_val(act_unit, FALSE); + + /* Sanity check: The target city still exists. */ + fc_assert_ret_val(tgt_city, FALSE); tgt_player = city_owner(tgt_city); - if (!tgt_player) { - /* How can a city be ownerless? */ - fc_assert(tgt_player); - - return FALSE; - } - - if (!act_unit) { - /* Actor unit was destroyed during pre action Lua. */ - return FALSE; - } + + /* How can a city be ownerless? */ + fc_assert_ret_val(tgt_player, FALSE); /* Save city ID. */ tgt_city_id = tgt_city->id; @@ -3202,15 +3187,12 @@ { const char *work; - if (NULL == punit) { - /* Probably died or bribed. */ - return FALSE; - } + /* Sanity check: The actor still exists. */ + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(punit, FALSE); /* Sanity check: The target city still exists. */ - if (NULL == pcity_dest) { - return FALSE; - } + fc_assert_ret_val(pcity_dest, FALSE); pcity_dest->shield_stock += unit_build_shield_cost(punit); pcity_dest->caravan_shields += unit_build_shield_cost(punit); @@ -3302,14 +3284,12 @@ struct goods_type *goods; const char *goods_str; - if (NULL == punit) { - /* Probably died or bribed. */ - return FALSE; - } - - if (!pcity_dest) { - return FALSE; - } + /* Sanity check: The actor still exists. */ + fc_assert_ret_val(pplayer, FALSE); + fc_assert_ret_val(punit, FALSE); + + /* Sanity check: The target city still exists. */ + fc_assert_ret_val(pcity_dest, FALSE); pcity_homecity = player_city_by_number(pplayer, punit->homecity); _______________________________________________ Freeciv-commits mailing list Freeciv-commits@gna.org https://mail.gna.org/listinfo/freeciv-commits