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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Aug 21, 2001 at 01:53:04AM -0700, Trent Piepho wrote:
> On Tue, 21 Aug 2001, Raimar Falke wrote:
> > freeciv. The only other solution I found is doing the analog of a
> 
> The "solution" is to not write code that is full of bugs and references
> invalid pointers.  In the end, there really is no way to escape that.
> 
> > 1.1          (freeciv  01-May-98): [Currently all of the major entities - 
> > units, cities, players, contains
> > 1.1          (freeciv  01-May-98): an unique id. This id is really only 
> > required when a reference to an entity
> > 1.1          (freeciv  01-May-98): is to be serialized(saved or distributed 
> > over the net). However in the
> > 1.1          (freeciv  01-May-98): current code, the id is also used for 
> > comparing, looking up and in general
> > 1.2          (freeciv  05-Aug-98): referencing entities. This results in a 
> > lot of mess and unnecessary duplicate
> > 1.2          (freeciv  05-Aug-98): functions. Often when programming, one 
> > wonders if some function needs
> > 1.2          (freeciv  05-Aug-98): the id or a pointer when referring to an 
> > entity. -PU]
> > 
> > So this is a long known problem and should be fixed.
> 
> 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);
city.h:struct city *is_allied_city_tile(struct tile *ptile, int playerid);
city.h:struct city *is_non_attack_city_tile(struct tile *ptile, int playerid);
city.h:struct city *is_non_allied_city_tile(struct tile *ptile, int playerid);
unit.h:int ground_unit_transporter_capacity(int x, int y, int playerid);
unit.h:int missile_carrier_capacity(int x, int y, int playerid,
unit.h:int airunit_carrier_capacity(int x, int y, int playerid,
unit.h:struct unit *is_allied_unit_tile(struct tile *ptile, int playerid);
unit.h:struct unit *is_enemy_unit_tile(struct tile *ptile, int playerid);
unit.h:struct unit *is_non_allied_unit_tile(struct tile *ptile, int playerid);
unit.h:struct unit *is_non_attack_unit_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]);
}

> > So what people think about such a plan:
> >  - only pointers are passed around, no more ids
> 
> You need ids in two cases.  The first is when the data came off the network or
> a save game file.  The second is when you call a function that might free a
> unit/city pointed to by a local pointer.  C doesn't do GC, so you need to
> re-lookup the ID to see if your pointer is still valid.  All other uses of IDs
> are probably mistakes.

Ack.

> >  - every id-to-pointer method has to be replaced by a call to a
> >  "ensure_valid_*" method/macro
> >  - step after step such "ensure_valid_*" are removed are replaced by a
> >  call to "lax_ensure_valid_*" which is either a noop or a call to
> >  "ensure_valid_*" depending on some define
> 
> Are you sure you mean "replace" here?  Do you mean that ensure_valid_* should
> be added to all calls to find_*_by_id?  Both find_*_by_id functions return
> NULL if the id isn't around.  It is perfectly acceptable for this to happen,
> e.g. because of network latency.  Almost all code that looks up IDs needs
> check for the possibility of NULL being returned and handle it in some way.

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

it should be replaced by

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

I'm not sure how many constructs are in the code which match the
above. As Markus pointed out server/maphand.c:map_get_player_tile() is
one such thing.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "How about the new language C&? No, that's not 'c ampersand', 'c reference', 
  'reference to c' or 'c and'. It's pronounced 'campersand', to confuse the 
  hell out of people who are unfamiliar with it, and it will, of course, 
  have no pointers."
    -- Xazziri in comp.lang.c++ about C#