[Freeciv-Dev] Re: (PR#3446) Review results.

["andrearo@xxxxxxxxxxxx" <andrearo@xxxxxxxxxxxx>]

Thanks for the review, here is the next increment of the patch.

On Thu, 11 Sep 2003, Remi Bonnet wrote:
> +  packet.nscores = j;
> +  for (i = 0; i < j; i++) {
> +    packet.pop[i] = size[i].player->player_no;
> ....
> 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

Fixed.

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

Fixed.

> Other problem in the non-supported gui's:
> +  char buffer[65536];
> ....
> +  free(buffer);
>
> Freeing of a static array

Fixed.

> >4. What can be improved / simplified / done differently (list)?
> >
> >
> in the non-supported gui's:
> +  char buffer[65536];
> a bit large...

With 30 players, this can get very large, reduced it to 32768.

> 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];
> +  }
> ....
> So you can replace all row by stat

This is an array of arrays. It's done just like this many other places
in repodlgs.c, eg. economy_report_dialog_update().

> You can also use smaller types in packets. Only uint32 is used but many
> fields requires only uint8 or uint16

Fixed.


Happy bug-hunting :)

endgame_report3.diff.gz
Description: endgame_report3.diff.gz