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

["Remi Bonnet" <remi.bonnet@xxxxxxxxxxx>]

>
>
>I. Testing
>
>1. Was the patch tested (yes / no-elaborate)?
>  
>
yes

>2. Which client was used to testing the patch (N/A if it's a
>server-only patch)?
>  
>
gtk-2.0 (doesn't test gtk since gtk-2.0 failed)

>3. Does the patch work / do what it promises to (yes / no)?
>  
>
some major bugs avoid a correct behaviour

>==========================================================================
>
>II. Code correctness
>
>1. Does the patch adhere to the style guide (yes / no)?
>  
>
yes

>2. Is the patch well commented / easy to read (yes / no-elaborate)?
>  
>
yes. There isn't many comments but the code is really easy to read and 
the functions descriptions are here.

>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

>==========================================================================
>
>III. Verdict
>
>1. Should the patch be
>  a) rejected completely
>  b) split - elaborate
>  c) revised and resubmitted for review
>  d) revised and then committed
>  e) committed as it is
>  
>
c) I think that another review is required before commit

>==========================================================================
>
>IV Anonymity
>
>1. Normally the referee's work would be acknowledged in the commit log
>message (if the patch is committed).  Would you rather prefer to
>remain anonymous (yes/no)?
>
You can show my name. I'm not a wanted terrorist :-p