[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 Mon, 19 Jul 2004, Per Inge Mathisen wrote:

> <URL: http://rt.freeciv.org/Ticket/Display.html?id=8754 >
>
> On Mon, 19 Jul 2004, Vasco Alexandre da Silva Costa wrote:
> > > 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.
>
> Bad excuse. I think we agreed that this should be the case for all new
> code, but I am not sure if this got codified in the style guide yet.

It isn't a bad excuse when pre-existing code doesn't have comments on all
functions.

I always prefered reading code instead of comments for short functions anyway.
And good function declarations summarize what the function does 90% of the time.

I have seen over commented code. It is not easier to read. Comments work
best when used sparingly.

> > > 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. :-)
>
> What are the advantages of vectors here?

It is possible they may be faster because the memory is allocated
contiguously. They could also use less space because of no next/prev
pointers. But the "smart" growth of vectors may move things the other
way around spacewise. Without profiling, I can't give a definitive answer.

> Sounds better to me.
>
> > > 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".
>
> "City_Has_Building" and "Player_Has_Building"?

"CityHasBuilding" it is then. No underscores, because these aren't
traditionally used either in flag or effect types.

> BTW, a continent-wide uniqueness would be nice.

I think I will remove all over city range checks. Both these ways of
checking an effect are equivalent:

1) All city Temples have +2 bonus if that player has an Oracle.
2) An Oracle gives +2 bonus to all player cities with a Temple.

Hence over city range building checks are redundant. And given the current
code, 2) is probably faster anyway.

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