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

On 08/09/2007, Marko Lindqvist wrote:
>
>  Comment says that pplayer can be NULL to check if city/unit is in
> game at all. But that will return TRUE only for units which have no
> owner (owner also NULL).

 Patches.

 Only callers using NULL seem to be global observers.

 These are already passed autogame regression testing. I will do some
more testing with global observers, but currently consider these ready
for commit (24h from sending this mail)


 - ML

diff -Nurd -X.diff_ignore freeciv/common/player.c freeciv/common/player.c
--- freeciv/common/player.c	2007-09-14 14:51:37.000000000 +0300
+++ freeciv/common/player.c	2007-09-15 13:20:24.000000000 +0300
@@ -507,14 +507,27 @@
  return pointer to the city struct.  Else return NULL.
  Now always uses fast idex_lookup_city.
 
-  pplayer may be NULL in which case all cities will be considered.
+ pplayer may be NULL in which case all cities registered to
+ hash are considered - even those not currently owned by any
+ player. Callers expect this behavior.
 ***************************************************************/
 struct city *player_find_city_by_id(const struct player *pplayer,
 				    int city_id)
 {
+  /* We call idex directly. Should use game_find_city_by_id()
+   * instead? */
   struct city *pcity = idex_lookup_city(city_id);
 
-  return (pcity && city_owner(pcity) == pplayer) ? pcity : NULL;
+  if (!pcity) {
+    return NULL;
+  }
+
+  if (!pplayer || (city_owner(pcity) == pplayer)) {
+    /* Correct owner */
+    return pcity;
+  }
+
+  return NULL;
 }
 
 /***************************************************************
@@ -522,14 +535,27 @@
  return pointer to the unit struct.  Else return NULL.
  Uses fast idex_lookup_city.
 
-  pplayer may be NULL in which case all units will be considered.
+ pplayer may be NULL in which case all units registered to
+ hash are considered - even those not currently owned by any
+ player. Callers expect this behavior.
 ***************************************************************/
 struct unit *player_find_unit_by_id(const struct player *pplayer,
 				    int unit_id)
 {
+  /* We call idex directly. Should use game_find_unit_by_id()
+   * instead? */
   struct unit *punit = idex_lookup_unit(unit_id);
 
-  return (punit && punit->owner == pplayer) ? punit : NULL;
+  if (!punit) {
+    return NULL;
+  }
+
+  if (!pplayer || (punit->owner == pplayer)) {
+    /* Correct owner */
+    return punit;
+  }
+
+  return NULL;
 }
 
 /*************************************************************************
diff -Nurd -X.diff_ignore freeciv/common/unit.c freeciv/common/unit.c
--- freeciv/common/unit.c	2007-09-14 17:25:07.000000000 +0300
+++ freeciv/common/unit.c	2007-09-15 13:08:57.000000000 +0300
@@ -1535,9 +1535,8 @@
 **************************************************************************/
 bool unit_alive(int id)
 {
-  /* Check if unit exist for any player
-   * (this is just one fast idex_lookup_unit() ) */
-  if (player_find_unit_by_id(NULL, id)) {
+  /* Check if unit exist in game */
+  if (game_find_unit_by_number(id)) {
     return TRUE;
   }
 
diff -Nurd -X.diff_ignore freeciv/common/player.c freeciv/common/player.c
--- freeciv/common/player.c	2007-07-04 14:04:25.000000000 +0300
+++ freeciv/common/player.c	2007-09-15 13:20:49.000000000 +0300
@@ -419,14 +419,27 @@
  return pointer to the city struct.  Else return NULL.
  Now always uses fast idex_lookup_city.
 
-  pplayer may be NULL in which case all cities will be considered.
+ pplayer may be NULL in which case all cities registered to
+ hash are considered - even those not currently owned by any
+ player. Callers expect this behavior.
 ***************************************************************/
 struct city *player_find_city_by_id(const struct player *pplayer,
 				    int city_id)
 {
+  /* We call idex directly. Should use game_find_city_by_id()
+   * instead? */
   struct city *pcity = idex_lookup_city(city_id);
 
-  return (pcity && pcity->owner == pplayer) ? pcity : NULL;
+  if (!pcity) {
+    return NULL;
+  }
+ 
+  if (!pplayer || (city_owner(pcity) == pplayer)) {
+    /* Correct owner */
+    return pcity;
+  }
+ 
+  return NULL;
 }
 
 /***************************************************************
@@ -434,14 +447,27 @@
  return pointer to the unit struct.  Else return NULL.
  Uses fast idex_lookup_city.
 
-  pplayer may be NULL in which case all units will be considered.
+ pplayer may be NULL in which case all units registered to
+ hash are considered - even those not currently owned by any
+ player. Callers expect this behavior.
 ***************************************************************/
 struct unit *player_find_unit_by_id(const struct player *pplayer,
 				    int unit_id)
 {
+  /* We call idex directly. Should use game_find_unit_by_id()
+   * instead? */
   struct unit *punit = idex_lookup_unit(unit_id);
 
-  return (punit && punit->owner == pplayer) ? punit : NULL;
+  if (!punit) {
+    return NULL;
+  }
+
+  if (!pplayer || (punit->owner == pplayer)) {
+    /* Correct owner */
+    return punit;
+  }
+
+  return NULL;
 }
 
 /*************************************************************************
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to