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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Aug 21, 2001 at 03:02:00AM -0700, Trent Piepho wrote:
> 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.

No. I thought the code contains more constructs like the above. I was
mistaken.

There are further optimizations possible:
 - what about passing struct tile * instead of (int x, int y)?
 - what about using pointers instead of ids in structs?

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Understanding is a three-edged sword; 
  your side, their side, and the truth."
    -- a well-known Vorlon