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

["Vasco Alexandre da Silva Costa" <vasc@xxxxxxxxxxxxxx>]

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

On Sun, 18 Jul 2004, Mike Kaufman wrote:

> <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.

Uh, this isn't true for the rest of the code, I don't see why the effects
code should be special in this regard.

> 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.

Ok, I'll add struct field comments.

> you can remove EFT_ALL ?

Yeah, thanks.

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

Yes, you are right.

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

I am ok with either, jason and per prefer genlists, you people slog
it out. :-)

> 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

Ok.

> 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()

This seems a bit verbose... How about:
get_buildings_with_effect()
get_building_effects()
get_building_effect_types()

?

This fits in with get_unit_upgrade_info, etc.

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

This could be done, but is it worth it? Assuming we have 400 effects, this
is 800 bytes overhead, less than 1 KB.

> 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.

That would be better from my POV as well, but the unfortunate user of
this function (the military AI) isn't adjusted for more than one building
of each type anyway. That would require rewriting the relevant military
AI code.

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

Ok, I can put it there. If it ends up being necessary for other functions
I can always remove it again.

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

Ok.

> 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);

Yes, it will work fine. The return will do a memcpy() of the struct,
and building_vector_free() frees the malloced array data.

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

That is a poor name really. I should have renamed it by now. "Building"
now means that it checks for the presence of that building in the city,
"Wonder" checks for the presence of that building in a player.

I could rename it to "CityBuilding" and "PlayerBuilding".

This actually ends up being the negation of "group" in the sense that it
stores a list of buildings (albeith with list length == 1) that are
required to be present in order for the effect to be active. Instead of
like "group" which is a list of buildings which must not be present in
order for the effect to be active.

The out of city ("Wonder") check is only used for the Oracle temple effect
anyway... Perhaps I should just dump it, and place the Oracle temple effect in
Oracle itself rather than on Temple. This actually makes more sense I
think, since University+Library also works this way. Yeah, I think I will
do just that.

---
Vasco Alexandre da Silva Costa @ Instituto Superior Tecnico, Lisboa