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

[Trent Piepho <xyzzy@xxxxxxxxxxxxx>]

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.

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

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