[Freeciv-Dev] Re: (PR#3446) Report at endgame

["Gregory Berkolaiko" <Gregory.Berkolaiko@xxxxxxxxxxxx>]

First of all, thanks a lot to Remi for a thorough review.

Andreas, when resubmitting the patch, please make sure you answer all 
Remi's points.  If he misunderstood some of your code, make sure you 
comment the code better.

Best wishes,
G.

On Thu, 11 Sep 2003, Remi Bonnet wrote:

> >3. Are there coding errors in the patch (list)?
> >  
> >
> yes
> 
> +  qsort(size, j, sizeof(struct player_score_entry), secompare);
> +
> +  packet.nscores = j;
> +  for (i = 0; i < j; i++) {
> +    packet.pop[i] = size[i].player->player_no;
> +    packet.score[i] = size[i].value;
> +    packet.pop[i] = get_pop(size[i].player);
> +    packet.bnp[i] = get_economics(size[i].player);
> +    packet.mfg[i] = get_production(size[i].player);
> +    packet.cities[i] = get_cities(size[i].player);
> +    packet.techs[i] = get_techs(size[i].player);
> +    packet.mil_service[i] = get_mil_service(size[i].player);
> +    packet.wonders[i] = get_wonders(size[i].player);
> +    packet.research[i] = get_research(size[i].player);
> +    packet.landarea[i] = get_landarea(size[i].player);
> +    packet.settledarea[i] = get_settledarea(size[i].player);
> +    packet.literacy[i] = get_literacy(size[i].player);
> +    packet.spaceship[i] = get_spaceship(size[i].player);
> +  }  
> 
> here packet.id[i] is unizialized and packet.pop[i] is initialized two 
> time (looks like the first one should be a packet.id[i] init
> 
> then in gtk-2.0,
> +    gtk_list_store_set(scores_store, &it,
> +                       0, (gchar *)get_player(i)->name,
> +                       1, packet->score[i],
> 
> and in gtk,
> +  for (i = 0; i < packet->nscores; i++) {
> +    row[0] = get_player(i)->name;
> +    my_snprintf(stat[1], sizeof(stat[1]), "%d", packet->score[i]);
>  
> the code assumes that the player id is the rank of the player in the 
> packet. But it is wrong! the packet is not sort by id but by score!
> 
> 
> Other problem in the non-supported gui's:
> +  char buffer[65536];
> ....
> +  free(buffer);
> 
> Freeing of a static array
> 
> >4. What can be improved / simplified / done differently (list)?
> >  
> >
> in the non-supported gui's:
> +  char buffer[65536];
> a bit large...
> 
> in the gui-gtk:
> row is useless: it is created with stat:
> 
> +  gchar *row[NUM_SCORE_COLS];
> +  char stat[NUM_SCORE_COLS][64];
> 
> then points to the same memory:
> 
> +  for (i = 0; i < ARRAY_SIZE(row); i++) {
> +    row[i] = stat[i];
> +  }     
> 
> then stat and row are used for the same purpose (comments added by myself)
> 
> +  for (i = 0; i < packet->nscores; i++) {
> +    row[0] = get_player(i)->name;                                       
>                /* First row is used */
> +    my_snprintf(stat[1], sizeof(stat[1]), "%d", 
> packet->score[i]);         /* Then stat */
> +    my_snprintf(stat[2], sizeof(stat[2]), "%d", packet->pop[i]);
> +    my_snprintf(stat[3], sizeof(stat[3]), "%d", packet->bnp[i]);
> +    my_snprintf(stat[4], sizeof(stat[4]), "%d", packet->mfg[i]);
> +    my_snprintf(stat[5], sizeof(stat[5]), "%d", packet->cities[i]);
> +    my_snprintf(stat[6], sizeof(stat[6]), "%d", packet->techs[i]);
> +    my_snprintf(stat[7], sizeof(stat[7]), "%d", packet->mil_service[i]);
> +    my_snprintf(stat[8], sizeof(stat[8]), "%d", packet->wonders[i]);
> +    my_snprintf(stat[9], sizeof(stat[9]), "%d", packet->research[i]);
> +    my_snprintf(stat[10], sizeof(stat[10]), "%d", packet->landarea[i]);
> +    my_snprintf(stat[11], sizeof(stat[11]), "%d", packet->settledarea[i]);
> +    my_snprintf(stat[12], sizeof(stat[12]), "%d", packet->literacy[i]);
> +    my_snprintf(stat[13], sizeof(stat[13]), "%d", packet->spaceship[i]);
> +    gtk_clist_append(GTK_CLIST(scores_list), row);                     
>    /* And row at the end */
> + }
> 
> So you can replace all row by stat
> 
> You can also use smaller types in packets. Only uint32 is used but many 
> fields requires only uint8 or uint16
>