[Freeciv-Dev] Re: (PR#9878) new function building_has_effect

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=9878 >

Gregory Berkolaiko wrote:

> 1. The reason I put fc_types.h before any other includes is to indicate
> that ultimately a header file should only include fc_types.h at most.  But
> maybe it's a wrong goal.

A header file should include everything it needs to compile.  So if a .c 
file includes this header file only it need include no other headers. 
This is Raimar's argument to which I agree.

fc_types.h is needed to avoid circular dependencies.  But once it exists 
we might as well add more stuff to it.  But we shouldn't add anything 
that requires including another header.

> 2. I assume building_has_effect will be extended soon.  So it will become
> a switch and we can begin with a switch to save future diff lines.

Whatever.

>  Note
> that it returns 0 whereas it should return bool.

True.

> 3. Is this area controversial at all?  Because otherwise I would just
> commit it.

I don't think there should be any issues.  I took the least 
controversial part of the effects patch I could find.  I wondered why 
the function returned a boolean unlike all the other functions that 
return an integer, at first this is very confusing.  So when figured it 
out I documented it.

The only possible question is over the implementation of 
building_has_effect.  It would be pretty easy to actually give this a 
real implementation that looped over effects to check them.  I believe 
the code for that is already in place.  But this is O(n) which is 
improved on by the caching done in Vasco's patch (so it's O(1)).  Of 
course in most cases n==1 anyway.

jason

Index: client/citydlg_common.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/citydlg_common.c,v
retrieving revision 1.43
diff -u -r1.43 citydlg_common.c
--- client/citydlg_common.c     28 Aug 2004 19:15:39 -0000      1.43
+++ client/citydlg_common.c     31 Aug 2004 05:15:18 -0000
@@ -281,7 +281,7 @@
                                     int id, bool is_unit,
                                     struct city *pcity)
 {
-  if (!is_unit && id == B_CAPITAL) {
+  if (!is_unit && building_has_effect(id, EFT_PROD_TO_GOLD)) {
     my_snprintf(buffer, buffer_len, _("%s (XX) %d/turn"),
                get_impr_name_ex(pcity, id), MAX(0, pcity->shield_surplus));
   } else {
@@ -333,7 +333,7 @@
     my_snprintf(buf[2], column_size, "%d", unit_build_shield_cost(id));
   } else {
     /* Total & turns left meaningless on capitalization */
-    if (id == B_CAPITAL) {
+    if (building_has_effect(id, EFT_PROD_TO_GOLD)) {
       my_snprintf(buf[0], column_size, get_improvement_type(id)->name);
       buf[1][0] = '\0';
       my_snprintf(buf[2], column_size, "---");
@@ -365,7 +365,7 @@
 
   /* Add the turns-to-build entry in the 4th position */
   if (pcity) {
-    if (!is_unit && id == B_CAPITAL) {
+    if (!is_unit && building_has_effect(id, EFT_PROD_TO_GOLD)) {
       my_snprintf(buf[3], column_size, _("%d/turn"),
                  MAX(0, pcity->shield_surplus));
     } else {
Index: client/climisc.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/climisc.c,v
retrieving revision 1.136
diff -u -r1.136 climisc.c
--- client/climisc.c    28 Aug 2004 19:15:39 -0000      1.136
+++ client/climisc.c    31 Aug 2004 05:15:18 -0000
@@ -559,7 +559,7 @@
  struct items and also sort it.
 
  section 0: normal buildings
- section 1: B_CAPITAL
+ section 1: Capitalization
  section 2: F_NONMIL units
  section 3: other units
  section 4: wonders
@@ -583,7 +583,7 @@
       pitem->section = unit_type_flag(id, F_NONMIL) ? 2 : 3;
     } else {
       name = get_impr_name_ex(pcity, id);
-      if (id == B_CAPITAL) {
+      if (building_has_effect(id, EFT_PROD_TO_GOLD)) {
        cost = -1;
        pitem->section = 1;
       } else {
Index: common/effects.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/effects.c,v
retrieving revision 1.6
diff -u -r1.6 effects.c
--- common/effects.c    28 Aug 2004 19:15:39 -0000      1.6
+++ common/effects.c    31 Aug 2004 05:15:19 -0000
@@ -215,6 +215,31 @@
 }
 
 /**************************************************************************
+  Returns TRUE if the building has any effect bonuses of the given type.
+
+  Note that this function returns a boolean rather than an integer value
+  giving the exact bonus.  Finding the exact bonus requires knowing the
+  effect range and may take longer.  This function should only be used
+  in situations where the range doesn't matter.
+
+  TODO:
+    1.  This function does not access the effect data directly; instead
+        it just associates the effect with a building.
+    2.  Only a few effects are supported.
+**************************************************************************/
+bool building_has_effect(Impr_Type_id building, enum effect_type effect)
+{
+  switch (effect) {
+  case EFT_PROD_TO_GOLD:
+    return building == B_CAPITAL;
+  default:
+    break;
+  }
+  assert(0);
+  return FALSE;
+}
+
+/**************************************************************************
   Returns the effect bonus the currently-in-construction-item will provide.
 
   Note this is not called get_current_production_bonus because that would
Index: common/effects.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/effects.h,v
retrieving revision 1.4
diff -u -r1.4 effects.h
--- common/effects.h    28 Aug 2004 19:15:39 -0000      1.4
+++ common/effects.h    31 Aug 2004 05:15:19 -0000
@@ -132,6 +132,7 @@
 bool are_effects_equal(const struct impr_effect *const peff1,
                       const struct impr_effect *const peff2);
 
+bool building_has_effect(Impr_Type_id building, enum effect_type effect);
 int get_current_construction_bonus(const struct city *pcity,
                                   enum effect_type effect);
 
Index: common/fc_types.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/fc_types.h,v
retrieving revision 1.3
diff -u -r1.3 fc_types.h
--- common/fc_types.h   28 Aug 2004 19:15:39 -0000      1.3
+++ common/fc_types.h   31 Aug 2004 05:15:19 -0000
@@ -21,6 +21,25 @@
 typedef signed short Continent_id;
 typedef enum tile_terrain_type Terrain_type_id;
 
+/* TODO: Remove this enum and make this an integer when gen-eff is done. */
+typedef enum {
+  B_AIRPORT=0, B_AQUEDUCT, B_BANK, B_BARRACKS, B_BARRACKS2, B_BARRACKS3, 
+  B_CATHEDRAL, B_CITY, B_COASTAL, B_COLOSSEUM, B_COURTHOUSE,  B_FACTORY, 
+  B_GRANARY, B_HARBOUR, B_HYDRO, B_LIBRARY, B_MARKETPLACE, B_MASS, B_MFG, 
+  B_NUCLEAR, B_OFFSHORE, B_PALACE, B_POLICE, B_PORT, B_POWER,
+  B_RECYCLING, B_RESEARCH, B_SAM, B_SDI, B_SEWER, B_SOLAR, B_SCOMP, 
+  B_SMODULE, B_SSTRUCTURAL, B_STOCK, B_SUPERHIGHWAYS, B_SUPERMARKET, B_TEMPLE,
+  B_UNIVERSITY,  
+  
+  B_APOLLO, B_ASMITHS, B_COLLOSSUS, B_COPERNICUS, B_CURE, B_DARWIN, B_EIFFEL,
+  B_GREAT, B_WALL, B_HANGING, B_HOOVER, B_ISAAC, B_BACH, B_RICHARDS, 
+  B_LEONARDO, B_LIGHTHOUSE, B_MAGELLAN, B_MANHATTEN, B_MARCO, B_MICHELANGELO, 
+  B_ORACLE, B_PYRAMIDS, B_SETI, B_SHAKESPEARE, B_LIBERTY, B_SUNTZU, 
+  B_UNITED, B_WOMENS,
+  B_CAPITAL, B_LAST_ENUM
+} Impr_Type_id;
+
+
 struct city;
 
 #endif /* FC__FC_TYPES_H */
Index: common/improvement.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/improvement.c,v
retrieving revision 1.43
diff -u -r1.43 improvement.c
--- common/improvement.c        13 Aug 2004 15:59:12 -0000      1.43
+++ common/improvement.c        31 Aug 2004 05:15:19 -0000
@@ -177,7 +177,7 @@
   int cost = 0, missing =
       improvement_types[id].build_cost - shields_in_stock;
 
-  if (id == B_CAPITAL) {
+  if (building_has_effect(id, EFT_PROD_TO_GOLD)) {
     /* Can't buy capitalization. */
     return 0;
   }
Index: common/improvement.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/improvement.h,v
retrieving revision 1.30
diff -u -r1.30 improvement.h
--- common/improvement.h        31 Aug 2004 04:40:50 -0000      1.30
+++ common/improvement.h        31 Aug 2004 05:15:19 -0000
@@ -15,13 +15,13 @@
 
 /* City Improvements, including Wonders.  (Alternatively "Buildings".) */
 
-#include "fc_types.h"
-
 #include "shared.h"            /* MAX_LEN_NAME */
+
+#include "effects.h"
+#include "fc_types.h"
 #include "tech.h"              /* Tech_Type_id */
 #include "terrain.h"           /* Terrain_type_id etc */
 #include "unittype.h"          /* Unit_Class_id, Unit_Type_id */
-#include "effects.h"
 
 struct player;
 
@@ -35,32 +35,6 @@
 #define I_REDUNDANT  3   /* Built, but replaced by wonder/other building */
 
 
-/* FIXME: Remove this define when there is per-file need for this enum. */
-#define OLD_IMPR_TYPE_ENUM
-
-/* FIXME: Remove this enum and the ifdef/endif when gen-impr implemented. */
-#ifdef OLD_IMPR_TYPE_ENUM
-enum improvement_type_id {
-  B_AIRPORT=0, B_AQUEDUCT, B_BANK, B_BARRACKS, B_BARRACKS2, B_BARRACKS3, 
-  B_CATHEDRAL, B_CITY, B_COASTAL, B_COLOSSEUM, B_COURTHOUSE,  B_FACTORY, 
-  B_GRANARY, B_HARBOUR, B_HYDRO, B_LIBRARY, B_MARKETPLACE, B_MASS, B_MFG, 
-  B_NUCLEAR, B_OFFSHORE, B_PALACE, B_POLICE, B_PORT, B_POWER,
-  B_RECYCLING, B_RESEARCH, B_SAM, B_SDI, B_SEWER, B_SOLAR, B_SCOMP, 
-  B_SMODULE, B_SSTRUCTURAL, B_STOCK, B_SUPERHIGHWAYS, B_SUPERMARKET, B_TEMPLE,
-  B_UNIVERSITY,  
-  
-  B_APOLLO, B_ASMITHS, B_COLLOSSUS, B_COPERNICUS, B_CURE, B_DARWIN, B_EIFFEL,
-  B_GREAT, B_WALL, B_HANGING, B_HOOVER, B_ISAAC, B_BACH, B_RICHARDS, 
-  B_LEONARDO, B_LIGHTHOUSE, B_MAGELLAN, B_MANHATTEN, B_MARCO, B_MICHELANGELO, 
-  B_ORACLE, B_PYRAMIDS, B_SETI, B_SHAKESPEARE, B_LIBERTY, B_SUNTZU, 
-  B_UNITED, B_WOMENS,
-  B_CAPITAL, B_LAST_ENUM
-};
-typedef enum improvement_type_id Impr_Type_id;
-#else
-typedef int Impr_Type_id;
-#endif
-
 /* B_LAST is a value which is guaranteed to be larger than all
  * actual Impr_Type_id values.  It is used as a flag value;
  * it can also be used for fixed allocations to ensure ability
Index: server/diplomats.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/diplomats.c,v
retrieving revision 1.58
diff -u -r1.58 diplomats.c
--- server/diplomats.c  26 Jul 2004 19:52:02 -0000      1.58
+++ server/diplomats.c  31 Aug 2004 05:15:23 -0000
@@ -1031,7 +1031,7 @@
      * City Walls, then there is a 50% chance of getting caught.
      */
     vulnerability = get_improvement_type(improvement)->sabotage;
-    if (city_got_building(pcity, B_CAPITAL)) {
+    if (city_got_building(pcity, B_PALACE)) {
       vulnerability /= 2;
     }
     if (myrand(100) >= vulnerability) {