[Freeciv-Dev] Re: [PATCH] Small improvement at genlist and maphand

[Trent Piepho <xyzzy@xxxxxxxxxxxxx>]

On Tue, 21 Aug 2001, Raimar Falke wrote:
> > That part is very old, older than the CVS date would have you believe.  IDs
> > used to be used a lot more, but all that was cleaned up.  I don't think you
> > are going to find many, if any, places were an ID is used where it 
> > shouldn't.
> 
> Quick grep shows in common:
> 
> city.h:struct city *is_enemy_city_tile(struct tile *ptile, int playerid);
> 
> All these are because of:
>   int players_allied(const int player, const int player2);
> And surprise, surprise:
> int players_allied(const int player, const int player2)
> {
>   return pplayers_allied(&game.players[player], &game.players[player2]);
> }

Heh, that's pretty stupid isn't it?  It's because those functions predate
alliances, so it used to be that you would do if(punit->owner == playerid) and
didn't need the player struct.  In other words, before all you would do is
compare player ids, so there was never any need to look at the player
structure.

I see that all these things are from player ids, not unit or city ids.  The
reason is that units and city struct are allocated dynamically.  It used to be
that find_unit_by_id would do a linear search through the list of all units to
find the unit.  This was too slow, so a index was used to speed up find_city,
then later generalized and used for find_unit too.

The player list on the other hand was a static array.  If you wanted to get
the struct for player n, you just did players[playerid] and that was it.  It
was later that get_player() was written.  It seems that switching from IDs to
pointers for players isn't done yet, but I agree it would be a good idea.

> If there is code like 
>  void foo(int id,...)
>  {
>     struct object *pobject=find_obbject_by_id(id);
>     ...
>  }

Except for player ids, all the code that does this should have a reason for
it.  Either the ID just came in off the network/savefile or it's used to avoid
dangling pointers.

>  void foo(struct object *pobject,...)
>  {
>    ensure_valid_object(pobject);
>    ...  
>  }

Do you really think it's necessary for every function to check all its pointer
arguments?  You talking like a thousand checks added to the code, to nearly
every single function, to check all the punit and pplayer and pcity arguments.
IMHO that's defensive programming going a little overboard.