Re: [Freeciv-Dev] (PR#40207) [PATCH] Should cities with Supermarket automatically get farmland benefit?

2008-05-18 Thread jacobn+freeci...@chiark.greenend.org.uk

URL: http://bugs.freeciv.org/Ticket/Display.html?id=40207 

 Can you make version for other branches (S2_2 and TRUNK) too?

Attached is a straight-ish port to S2_2 (auto-farmland-ter-2-2.diff). It
is functional, but I'm unhappy with it, as it's no longer robust :(

On S2_2, various tests (e.g., is_city_center()) have been modified to
use pointer comparison on (struct tile *), in change 14416 (PR#40104).
This means that p_dummy_tile can't be passed to such functions; while
I've defused the two instances that appear directly in
city_tile_output(), thus making the patch apparently functional, and I
haven't found any instances in currently called functions, this code is
not robust against invocations of is_city_center() and similar appearing
in future (e.g., deep within the effects system).

I don't see an easy way out of this, unless the policy of using pointer
comparison is reversed. Temporarily modifying the tile in-place seems
evil (especially as the comments for city_tile_output() make it clear
that it can be called speculatively).

I shan't attempt to port to the trunk until this issue with S2_2 is
resolved.

Index: common/city.c
===
--- common/city.c   (revision 14693)
+++ common/city.c   (working copy)
@@ -710,9 +710,12 @@
 int city_tile_output(const struct city *pcity, const struct tile *ptile,
 bool is_celebrating, Output_type_id otype)
 {
-  struct tile tile;
+  struct tile dummy_tile = *ptile;  /* temporary dummy tile for city
+ * center bonuses */
+  const struct tile *p_dummy_tile = dummy_tile;
+/* drop-in ptile replacement */
   int prod;
-  struct terrain *pterrain = tile_terrain(ptile);
+  struct terrain *pterrain = tile_terrain(p_dummy_tile);
 
   assert(otype = 0  otype  O_LAST);
 
@@ -723,42 +726,41 @@
   }
 
   prod = pterrain-output[otype];
-  if (tile_resource_is_valid(ptile)) {
-prod += tile_resource(ptile)-output[otype];
+  if (tile_resource_is_valid(p_dummy_tile)) {
+prod += tile_resource(p_dummy_tile)-output[otype];
   }
 
-  /* create dummy tile which has the city center bonuses. */
-  tile.terrain = pterrain;
-  tile.special = tile_specials(ptile);
-
+  /* add city center bonuses to temporary copy of tile. */
   if (NULL != pcity
-   is_city_center(pcity, ptile)
+   is_city_center(pcity, ptile) /* relies on pointer comparison */
pterrain == pterrain-irrigation_result
terrain_control.may_irrigate) {
-/* The center tile is auto-irrigated. */
-tile_set_special(tile, S_IRRIGATION);
+/* The center tile is auto-irrigated, for the purposes of food
+ * production. */
+tile_set_special(dummy_tile, S_IRRIGATION);
 
 if (player_knows_techs_with_flag(city_owner(pcity), TF_FARMLAND)) {
-  tile_set_special(tile, S_FARMLAND);
+  tile_set_special(dummy_tile, S_FARMLAND);
 }
   }
 
   switch (otype) {
   case O_SHIELD:
-if (contains_special(tile.special, S_MINE)) {
+if (tile_has_special(p_dummy_tile, S_MINE)) {
   prod += pterrain-mining_shield_incr;
 }
 break;
   case O_FOOD:
-if (contains_special(tile.special, S_IRRIGATION)) {
+if (tile_has_special(p_dummy_tile, S_IRRIGATION)) {
   prod += pterrain-irrigation_food_incr;
 }
 break;
   case O_TRADE:
-if (contains_special(tile.special, S_RIVER)  !is_ocean(tile.terrain)) {
+if (tile_has_special(p_dummy_tile, S_RIVER)
+ !is_ocean_tile(p_dummy_tile)) {
   prod += terrain_control.river_trade_incr;
 }
-if (contains_special(tile.special, S_ROAD)) {
+if (tile_has_special(p_dummy_tile, S_ROAD)) {
   prod += pterrain-road_trade_incr;
 }
 break;
@@ -769,28 +771,29 @@
 break;
   }
 
-  if (contains_special(tile.special, S_RAILROAD)) {
+  if (tile_has_special(p_dummy_tile, S_RAILROAD)) {
 prod += (prod * terrain_control.rail_tile_bonus[otype]) / 100;
   }
 
   if (pcity) {
 const struct output_type *output = output_types[otype];
 
-prod += get_city_tile_output_bonus(pcity, ptile, output,
+prod += get_city_tile_output_bonus(pcity, p_dummy_tile, output,
   EFT_OUTPUT_ADD_TILE);
 if (prod  0) {
-  int penalty_limit = get_city_tile_output_bonus(pcity, ptile, output,
+  int penalty_limit = get_city_tile_output_bonus(pcity, p_dummy_tile,
+   output,
EFT_OUTPUT_PENALTY_TILE);
 
   if (is_celebrating) {
-prod += get_city_tile_output_bonus(pcity, ptile, output,
+prod += get_city_tile_output_bonus(pcity, p_dummy_tile, output,
EFT_OUTPUT_INC_TILE_CELEBRATE);
 penalty_limit = 0; /* no penalty if celebrating */
   }
-  prod += get_city_tile_output_bonus(pcity, ptile, output,
+  prod += 

Re: [Freeciv-Dev] (PR#40207) [PATCH] Should cities with Supermarket automatically get farmland benefit?

2008-05-03 Thread [EMAIL PROTECTED]

URL: http://bugs.freeciv.org/Ticket/Display.html?id=40207 

Here's a new patch which is functionally equivalent to my previous one
but includes Madeline's suggestion:

 - rename tile to dummy_tile throughout

and also includes the more controversial changes mentioned previously,
such that I'm not sure whether or not it improves the chances of my
patch being incorporated. Take your pick ;)

 - introduce and use a const p_dummy_tile; this attempts to mitigate the
   pointer aliasing introduced by my change, but the result looks a
   little awkward;

 - use the standard tile_has_special() function and the like instead of
   the current approach of reaching inside struct tile, at the
   potential run-time cost of introducing function calls.

--- freeciv-2.1.4-orig/common/city.c2008-04-21 01:10:49.0 +0100
+++ freeciv-2.1.4/common/city.c 2008-05-03 12:57:03.0 +0100
@@ -607,9 +607,11 @@
int city_x, int city_y, bool is_celebrating,
Output_type_id otype)
 {
-  struct tile tile;
+  /* temporary dummy tile for city center bonuses */
+  struct tile dummy_tile = *ptile;
+  const struct tile *p_dummy_tile = dummy_tile;
   int prod;
-  struct terrain *pterrain = tile_get_terrain(ptile);
+  struct terrain *pterrain = tile_get_terrain(p_dummy_tile);
 
   assert(otype = 0  otype  O_LAST);
 
@@ -620,41 +622,40 @@
   }
 
   prod = pterrain-output[otype];
-  if (tile_resource_is_valid(ptile)) {
-prod += tile_get_resource(ptile)-output[otype];
+  if (tile_resource_is_valid(p_dummy_tile)) {
+prod += tile_get_resource(p_dummy_tile)-output[otype];
   }
 
-  /* create dummy tile which has the city center bonuses. */
-  tile.terrain = pterrain;
-  tile.special = tile_get_special(ptile);
-
+  /* add city center bonuses to temporary copy of tile. */
   if (pcity  is_city_center(city_x, city_y)
pterrain == pterrain-irrigation_result
terrain_control.may_irrigate) {
-/* The center tile is auto-irrigated. */
-tile_set_special(tile, S_IRRIGATION);
+/* The center tile is auto-irrigated, for the purposes of food
+ * production. */
+tile_set_special(dummy_tile, S_IRRIGATION);
 
 if (player_knows_techs_with_flag(city_owner(pcity), TF_FARMLAND)) {
-  tile_set_special(tile, S_FARMLAND);
+  tile_set_special(dummy_tile, S_FARMLAND);
 }
   }
 
   switch (otype) {
   case O_SHIELD:
-if (contains_special(tile.special, S_MINE)) {
+if (tile_has_special(p_dummy_tile, S_MINE)) {
   prod += pterrain-mining_shield_incr;
 }
 break;
   case O_FOOD:
-if (contains_special(tile.special, S_IRRIGATION)) {
+if (tile_has_special(p_dummy_tile,  S_IRRIGATION)) {
   prod += pterrain-irrigation_food_incr;
 }
 break;
   case O_TRADE:
-if (contains_special(tile.special, S_RIVER)  !is_ocean(tile.terrain)) {
+if (tile_has_special(p_dummy_tile, S_RIVER) 
+!is_ocean(tile_get_terrain(p_dummy_tile))) {
   prod += terrain_control.river_trade_incr;
 }
-if (contains_special(tile.special, S_ROAD)) {
+if (tile_has_special(p_dummy_tile, S_ROAD)) {
   prod += pterrain-road_trade_incr;
 }
 break;
@@ -665,28 +666,29 @@
 break;
   }
 
-  if (contains_special(tile.special, S_RAILROAD)) {
+  if (tile_has_special(p_dummy_tile, S_RAILROAD)) {
 prod += (prod * terrain_control.rail_tile_bonus[otype]) / 100;
   }
 
   if (pcity) {
 const struct output_type *output = output_types[otype];
 
-prod += get_city_tile_output_bonus(pcity, ptile, output,
+prod += get_city_tile_output_bonus(pcity, p_dummy_tile, output,
   EFT_OUTPUT_ADD_TILE);
 if (prod  0) {
-  int penalty_limit = get_city_tile_output_bonus(pcity, ptile, output,
-   EFT_OUTPUT_PENALTY_TILE);
+  int penalty_limit = get_city_tile_output_bonus(pcity, p_dummy_tile,
+ output,
+ EFT_OUTPUT_PENALTY_TILE);
 
   if (is_celebrating) {
-prod += get_city_tile_output_bonus(pcity, ptile, output,
+prod += get_city_tile_output_bonus(pcity, p_dummy_tile, output,
EFT_OUTPUT_INC_TILE_CELEBRATE);
 penalty_limit = 0; /* no penalty if celebrating */
   }
-  prod += get_city_tile_output_bonus(pcity, ptile, output,
+  prod += get_city_tile_output_bonus(pcity, p_dummy_tile, output,
  EFT_OUTPUT_INC_TILE);
   prod += (prod 
-   * get_city_tile_output_bonus(pcity, ptile, output,
+   * get_city_tile_output_bonus(pcity, p_dummy_tile, output,
 EFT_OUTPUT_PER_TILE)) 
   / 100;
   if (!is_celebrating  penalty_limit  0  prod  penalty_limit) {
@@ -695,11 +697,11 @@
 }
   }
 
-  if (contains_special(tile.special, 

Re: [Freeciv-Dev] (PR#40207) [PATCH] Should cities with Supermarket automatically get farmland benefit?

2008-04-26 Thread [EMAIL PROTECTED]

URL: http://bugs.freeciv.org/Ticket/Display.html?id=40207 

Since at least one other person seems to think it's a real bug, I've
investigated further.

Looking further down the function, it becomes obvious why the temporary
setting of S_FARMLAND isn't having any effect while S_IRRIGATION is: the
effects of irrigation are built-in and the checking code uses the
dummy tile with the extra specials, whereas farmland is implemented
through the effects system, and it's the original ptile that gets
passed into that. So presumably this change was introduced whenever the
effects system came in.

To fix this, it's not safe to just pass tile as it stands into the
bowels of the effects system, since parts of that can access members of
tile that haven't been initialised. However, I can't see any downside
to making tile into a full copy of *ptile then passing that around,
since nothing called from here should be modifying *ptile or any of its
descendants.

The attached patch implements this. (I had to bracket a macro argument
in a header file as well.) It fixes the bug, and hasn't obviously messed
anything else up.

diff -uNr freeciv-2.1.4-orig/common/city.c freeciv-2.1.4/common/city.c
--- freeciv-2.1.4-orig/common/city.c2008-04-21 01:10:49.0 +0100
+++ freeciv-2.1.4/common/city.c 2008-04-26 21:27:50.0 +0100
@@ -607,9 +607,10 @@
int city_x, int city_y, bool is_celebrating,
Output_type_id otype)
 {
-  struct tile tile;
+  /* temporary dummy tile for city center bonuses */
+  struct tile tile = *ptile;
   int prod;
-  struct terrain *pterrain = tile_get_terrain(ptile);
+  struct terrain *pterrain = tile_get_terrain(tile);
 
   assert(otype = 0  otype  O_LAST);
 
@@ -620,18 +621,16 @@
   }
 
   prod = pterrain-output[otype];
-  if (tile_resource_is_valid(ptile)) {
-prod += tile_get_resource(ptile)-output[otype];
+  if (tile_resource_is_valid(tile)) {
+prod += tile_get_resource(tile)-output[otype];
   }
 
-  /* create dummy tile which has the city center bonuses. */
-  tile.terrain = pterrain;
-  tile.special = tile_get_special(ptile);
-
+  /* add city center bonuses to temporary copy of tile. */
   if (pcity  is_city_center(city_x, city_y)
pterrain == pterrain-irrigation_result
terrain_control.may_irrigate) {
-/* The center tile is auto-irrigated. */
+/* The center tile is auto-irrigated, for the purposes of food
+ * production. */
 tile_set_special(tile, S_IRRIGATION);
 
 if (player_knows_techs_with_flag(city_owner(pcity), TF_FARMLAND)) {
@@ -672,21 +671,21 @@
   if (pcity) {
 const struct output_type *output = output_types[otype];
 
-prod += get_city_tile_output_bonus(pcity, ptile, output,
+prod += get_city_tile_output_bonus(pcity, tile, output,
   EFT_OUTPUT_ADD_TILE);
 if (prod  0) {
-  int penalty_limit = get_city_tile_output_bonus(pcity, ptile, output,
+  int penalty_limit = get_city_tile_output_bonus(pcity, tile, output,
EFT_OUTPUT_PENALTY_TILE);
 
   if (is_celebrating) {
-prod += get_city_tile_output_bonus(pcity, ptile, output,
+prod += get_city_tile_output_bonus(pcity, tile, output,
EFT_OUTPUT_INC_TILE_CELEBRATE);
 penalty_limit = 0; /* no penalty if celebrating */
   }
-  prod += get_city_tile_output_bonus(pcity, ptile, output,
+  prod += get_city_tile_output_bonus(pcity, tile, output,
  EFT_OUTPUT_INC_TILE);
   prod += (prod 
-   * get_city_tile_output_bonus(pcity, ptile, output,
+   * get_city_tile_output_bonus(pcity, tile, output,
 EFT_OUTPUT_PER_TILE)) 
   / 100;
   if (!is_celebrating  penalty_limit  0  prod  penalty_limit) {
diff -uNr freeciv-2.1.4-orig/common/tile.h freeciv-2.1.4/common/tile.h
--- freeciv-2.1.4-orig/common/tile.h2008-04-21 01:10:49.0 +0100
+++ freeciv-2.1.4/common/tile.h 2008-04-26 21:27:48.0 +0100
@@ -74,7 +74,7 @@
 void tile_clear_special(struct tile *ptile, enum tile_special_type spe);
 void tile_clear_all_specials(struct tile *ptile);
 
-#define tile_resource_is_valid(vtile) BV_ISSET(vtile-special, 
S_RESOURCE_VALID)
+#define tile_resource_is_valid(vtile) BV_ISSET((vtile)-special, 
S_RESOURCE_VALID)
 const struct resource *tile_get_resource(const struct tile *ptile);
 void tile_set_resource(struct tile *ptile, struct resource *presource);
 
___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev