[Freeciv-Dev] Re: (PR#3521) Re: (PR#3601) Intelligence dialog empty

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

Per I. Mathisen wrote:
> On Tue, 4 Mar 2003, Per I. Mathisen wrote:
> 
>>On Mon, 3 Mar 2003, Per I. Mathisen wrote:
>>
>>>This fixes the symptom, and I think it can suffice for now:
>>
>>Well, here is another and better fix. Attached. Kudos to Vasco for coming
>>up with the solution.
> 
> 
> A third fix, kudos to Jason. It also works.
> 
> Index: client/packhand.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/client/packhand.c,v
> retrieving revision 1.293
> diff -u -r1.293 packhand.c
> --- client/packhand.c   2003/02/20 23:00:55     1.293
> +++ client/packhand.c   2003/03/04 02:30:33
> @@ -1309,7 +1309,7 @@
> 
>    pplayer->is_connected = pinfo->is_connected;
> 
> -  if (pplayer->is_connected) {
> +  if (game.num_tech_types > 0) {
>      read_player_info_techs(pplayer, pinfo->inventions);
>    }
> 
> Raimar, now you have lots of fixes to choose between!

After a little bit of thought, I think that initializing the data in 
player.c is a hack.  The thing is that only the first 
game.num_tech_types technologies are (supposed to be) guaranteed to have 
valid data, so to assert that the first (A_NONE) tech is TECH_KNOWN 
simply doesn't make sense if game.num_tech_types==0.  Moreover, spurious 
initialization may conceal other errors (like accessing invalid data) 
that could otherwise be more easily caught (by valgrind, for instance).

The above patch is one possible fix based on that idea.  It's also 
possible to put the check into read_player_info_techs, or to push it all 
the way into update_research and just change the assert to
   assert(game.num_tech_types == 0 || get_tech(...) == TECH_KNOWN));

But, none of this actually works because there *is* an actual bug here. 
  If you start a game with two human players, then the technologies will 
be read and every player's techs initialized (I think).  But if a player 
then disconnects, the player data will be sent out again without ever 
being (correctly) initialized.  In this case we'll still get an 
assertion under the above patch.  This is most likely harmless, because 
immediately after this the game restarts.  It does get fixed by 
initializing the data in player.c, but this still seems wrong to me.

Note valgrind does not report any uninitialized memory errors for 
civserver.  I guess all the player data is zeroed out, so if we do 
initialize it in player.c the memset is unnecessary and only A_NONE 
needs to be set.

jason