[Freeciv-Dev] Re: [FreeCiv-Cvs] thue: Move the seen field from the common map to the...

[David Pfitzner <dwp@xxxxxxxxxxxxxx>]

> Many functions take
> struct player *pplayer
> as argument when what they need is really the player id (for lookup in a
> bitvector). This patch corrects a few cases of that.

I'm a bit late on this, but will comment anyway...

This change seems to me a bit gratuitous: it results in widespread
code changes, yet is of limited desirability IMO.

I don't think there is any real efficiency argument, since it is
very simple to transform in either direction between pplayer and
playerid.

And the programmer convenience issue is relatively small, since it 
is trivial to do pplayer->player_no (hmm, that should be player_num 
IMO), or get_player(playerid) (which could be macro if wanted).

So I think the "standard" should be to pick the one which is more
"natural"/useful in most cases, and use that except where there is
clear reason to prefer the other.  To me this means the standard
should be pplayer.  Though I suspect Thue has been doing lots of 
stuff outside the player struct recenty, so finds playerid more 
natural...  But still I think most cases this can be hidden 
inside the map etc functions rather than having patches with lots
of changes where the caller has do the change, as:
-  else if( map_get_known_and_seen(xu, yu, victim) )
+  else if( map_get_known_and_seen(xu, yu, victim->player_no) )

One advantage of using pplayer more than playerid is that it 
provides some "type safety", in that its harder to accidently
use a unit_id or something, whereas playerid is just an int.

> Change the order of function argument from (player, x, y) to (x, y, player) in
> some functions for consistency. More functions need to be changed.

(This change also seems a bit unnecessary to me, without a more
clearly elaborated "consistency", but nevermind...)

Regards,
-- David