[Freeciv-Dev] Re: (PR#8754) effects patch

["Mike Kaufman" <kaufman@xxxxxxxxxxxxxxxxxxxxxx>]

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

here are a couple of things. by no means exhaustive...

all functions in effects.c should have comments in the their headers.

group -> eff_group
group_element -> eff_group_element
etc.

in the ruleset cache, there should be comments for each struct describing
what
it stores. The wrappers should have comments too.

you can remove EFT_ALL ?

you have eff_list   for struct effect
you have eff_vector for enum effect_type
this is confusing. use eft_vector instead.

why do you use genlists for eff_list? why not also vectors?

in ruleset cache rename
  struct {
    struct eff_vector effects;
  } buildings[B_LAST];
to
  struct {
    struct eff_vector types;
  } buildings[B_LAST];

make groups_id static

rename get_effect_buildings() -> get_buildings_with_effect()
rename get_building_effect() -> get_effects_of_building()
rename get_building_effects() -> get_effect_types_of_building()

you should be able to make effect.value a short with no problems.

in aicity.c:adjust_building_want_by_effects(),
  cities[EFR_LAST] is cached in ai_data. Use that instead.

I'm wondering if find_source_building() should return a building_vector,
then
use building_vector_size() when you need to check for any such building.

you have one user for eventual_source_power(). Is there a reason not to
fold it into get_current_construction_bonus()?

I don't like the term "power" in source_power(), etc. Use "value" instead.

I'm fairly sure that this is ok and structure copy will work here
but just reassure me that this is correct C?

 struct building_vector sources;
 ...
 sources = get_city_bonus_sources(pcity, EFT_MAKE_CONTENT);
 ...
 building_vector_free(&sources);


what does "Wonder" for req_type mean anyway, and how does this jibe with
uniqueness upgrade?

-mike