[Freeciv-Dev] Re: [Patch] Cleanup regarding {city,unit}_owner

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Aug 23, 2001 at 03:36:07PM -0700, Trent Piepho wrote:
> On Thu, 23 Aug 2001, Raimar Falke wrote:
> > The first patch changes constructs like "get_player(punit->owner)" to
> > "unit_owner(punit)". The second patch changes constructs like
> > "&game.players[punit->owner]" also to "unit_owner(punit)".
> 
> It looks like you changed city owner in the same way as unit owner, which
> makes sense.
> 
> But, why not just make the owner field a player pointer?  Then you could
> change:

This is the next step. First we increase the encapsulation.

> &game.players[punit->owner]
> or
> get_player(punit->owner)
> 
> to
> 
> punit->owner
> 
> More efficient, less code, less typing.  And if you want to remove IDs where
> they aren't needed, you're going to want to change the owner field to a
> pointer anyway.

Maybe.

> Now, I'm guessing you might say that unit_owner() can just be changed to
> #define UNIT_OWNER(punit) (punit->owner)
> 
> But, it is generally considered neither necessary, nor desirable, in C
> programming to create accessor functions for members of structures.  Accessor
> functions like:
> 
> struct player *get_player(int playerid);
> struct unit *get_unit_by_id(int id);
> 
> Are ok, because you don't know underlying data structure that player or unit
> structs are contained in.  So while get_player() may just be a one-expression
> array index, that could change.  get_unit in fact has changed, it used to be
> that it searched an array of lists for the unit, but that was changed to a
> hash table for speed.  I think it was Syela who first did that back when he
> wrote the AI.  But once you have the structure in hand, it is ok to just
> access its fields with ->.

You see with a function like {unit,city}_owner() we can
experiment. These functions doesn't harm.

> It would be bad to get into a situation where 95% of structure
> members are accessed with ->, and 5% have acessor functions for no
> apparent reason.

May plan is something like this:
 1) finish the cleanup
 2) profile (there are now a lot more calls to {unit,city}_owner()
 3) if {unit,city}_owner() is called too often and take too much time
 think about reducing the number of calls
 4) after this if {unit,city}_owner() still takes to much time make a
 macro/inline function out of it
 5) if somebody sends me a patch for making owner field a pointer and
 this is considerable faster than 4) I will apply it. In this patch
 {unit,city}_owner() would probably end up as macros/inline functions
 (but are still lower cased)

What do you think?

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "It is not yet possible to change operating system by writing
  to /proc/sys/kernel/ostype."              sysctl(2) man page