[Freeciv-Dev] Re: a plenty of bugs

[Raimar Falke <rf13@xxxxxxxxxxxxxxxxx>]

On Fri, Jun 14, 2002 at 02:15:29AM +0200, Sylvain Tricot wrote:
> /*************************************************************************/
> unused id "j" in /freeciv-cvs-Mar-27/client/gui-gtk/gotodlg.c
> 
> 198:
>     for(i=0, j=0; i<game.nplayers; i++) {

Will be fixed if we do another round of "replace-for loops with
iterate macros".

> /*************************************************************************/
> suspicious code in /freeciv-cvs-Mar-27/client/gui-gtk/dialogs.c
> 
> in function:
> void popup_incite_dialog(struct city *pcity)
> we find
> 984:
>      diplomat_target_id = pcity->id;
> 
> but in function:
> void popup_bribe_dialog(struct unit *punit)
> the following line is missing :(
>      diplomat_target_id = punit->id;
> 
> Is this deliberate. WHY?

diplomat_target_id is set in popup_diplomat_dialog. It looks like the
assignment in popup_incite_dialog can be removed.

> /*************************************************************************/
> suspicious code in /freeciv-cvs-Mar-27/intl/localealias.c
> 
> 42:
>    #pragma alloca
> 
> correction:
> # pragma alloca

Not our code.

> static void setup_isledata(void)
> .
> .
> .
> 1044:
>       if(mingood+1<maxgood/game.nplayers)
>         mingood= maxgood/game.nplayers;
> 
> correction:
>       if(mingood<maxgood/game.nplayers)  // +1 ??
>         mingood= maxgood/game.nplayers;
> 
> /*************************************************************************/
> suspicious code in /freeciv-cvs-Mar-27/server/mapgen.c
> 
> static void setup_isledata(void)
> .
> .
> .
> 1047:
>       if(goodisles>game.nPlayer()){
> correction:
>       if(goodisles<game.nPlayer()){ // seems more logical

No idea.

> /*************************************************************************/
> suspicious code in /freeciv-1.11.5/server/stdinhand.c
> 
> 
> 2592:
>     /* commands may be prefixed with SERVER_COMMAND_PREFIX,
>        even when given on the server command line - rp */
> 
>     if (*cptr_s == SERVER_COMMAND_PREFIX) cptr_s++;
> 
>     for(cptr_s=str; *cptr_s && !isalnum(*cptr_s); cptr_s++);
> 
> //cptr_s=str follows cptr_s++ ???

This is fixed in the current code.

> /*************************************************************************/
> suspicious code in /freeciv-1.11.5/client/tilespec.c
> 
> in
> static void tilespec_lookup_sprite_tags(void)
> 
> 
> 476:
> 603:
> 623:  for(i=1; i<NUM_DIRECTION_NSEW; i++) {
> /
> 580:
> 586:
> 591:
> 596:
> ...:  for(i=0; i<NUM_DIRECTION_NSEW; i++)
> correction?:
> ???

Isometric tilesets are index by 0 non-iso. ones by 1.

> /*************************************************************************/
> bug in /freeciv-1.11.5/ai/aiunit.c line 2295
> 
> static void ai_manage_barbarian_leader(struct player *pplayer, struct
> unit *leader)
> :
> :
> 2295:
> y = map_adjust_x(leader->y + dy);
> 
> correction:
> 2295:         y = map_adjust_y(leader->y + dy);

Old code.

> /*************************************************************************/
> bug in /freeciv-1.11.5/client/gui-xaw/helpdlg.c line 793
> 
>       xaw_set_label(help_improvement_req_data, _("(Never)"));
> 793: create_tech_tree(help_tech_tree, 0, game.num_tech_types, 3);
> 
> correction:
>        /* A_LAST means NEVER */
> 793: create_tech_tree(help_tech_tree, 0, A_LAST, 3);

These are equivalent. This is a matter of taste.

> /*************************************************************************/
> bug in /freeciv-1.11.5/common/registry.c line 439
> 
>    if (i<columns_tab.n) {
>      astr_minsize(&entry_name, base_name.n + 10 + columns[i].n);
>      my_snprintf(entry_name.str, entry_name.n_alloc, "%s%d.%s",
>           base_name.str, table_lineno, columns[i].str);
>    } else {
> 439:   astr_minsize(&entry_name, base_name.n + 20 + columns[i].n);
>      my_snprintf(entry_name.str, entry_name.n_alloc, "%s%d.%s,%d",
>           base_name.str, table_lineno,
>           columns[columns_tab.n-1].str, i-columns_tab.n+1);
>    }
> 
> correction:
> 439:   astr_minsize(&entry_name, base_name.n + 20 +
>                                             columns[columns_tab.n-1].n);

No idea.

> /*************************************************************************/
> bug in /freeciv-1.11.5/server/sernet.c line 432
> 
>         char buf[BUF_SIZE+1];
> :
> :
> 432:     if ((bufptr-buf) <= BUF_SIZE) bufptr++; /* prevent overrun */
> 
> correction:
> 432:     if ((bufptr-buf) < BUF_SIZE) bufptr++; /* prevent overrun */

No longer part of the code.

> /*************************************************************************/
> correction in /freeciv-1.11.5/server/ruleset.c/
> 
> function:
> static void load_ruleset_nations(struct section_file *file)
> 
> 1935:
>         if(val != A_LAST && val != A_NONE) {
>       freelog(LOG_DEBUG, "%s tech goal (%d) %3d %s", pl->name, j, val, 
> techs[j]);
>       pl->goals.tech[j] = val;
>         }
> 
> correction:
>         if(val != A_LAST && val != A_NONE) {
>       freelog(LOG_DEBUG, "%s tech goal (%d) %3d %s", pl->name, j, val, 
> techs[j]);
>         }
>         pl->goals.tech[j] = val;

Per?

> /*************************************************************************/
> suspicious declarations in /freeciv-1.11.5/client/gui-gtk/
> 
> 
> the following callback functions for "button_release_event" and
> "button_press_event" events
> 
>        gtk_signal_connect(GTK_OBJECT(pdialog->present_unit_boxes[i]),
>       "button_release_event",
>       GTK_SIGNAL_FUNC(p_units_middle_callback), (gpointer)punit->id);
> 
> 
> has different signatures:
> 
> gui-gtk/mapctrl.c  ("button_release_event")
> static gint popit_button_release(GtkWidget *w, GdkEventButton *event)
> 
> gui-gtk/mapctrl.c  ("button_press_event")
> gint butt_down_overviewcanvas(GtkWidget *w, GdkEventButton *ev)
> 
> gui-gtk/citydlg.c  ("button_release_event")
> gint p_units_middle_callback(GtkWidget *w, GdkEventButton *ev, gpointer
> data)
> 
> gui-gtk/dialogs.c ("button_press_event")
> void taxrates_callback( GtkWidget *w, GdkEventButton *event, gpointer data )
> 
> 
> and so on.
> 
> PS: it's OK but bad code :8(

Mike, Vasco?

> /*************************************************************************/
> bug in /freeciv-1.11.5/server/cityhand.c/
> 
> function:
> void handle_city_make_specialist(struct player *pplayer,
>                                struct packet_city_request *preq)
> 
> 329:
>     if (is_worker_here(pcity, preq->worker_x, preq->worker_y) ==
> C_TILE_WORKER) {
> 
> correction:
>    if (is_worker_here(pcity, preq->worker_x, preq->worker_y)) {

Old code.

Question: is this some script/program or have you combed the code by
hand?

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "We just typed make..."
    -- Stephen Lambrigh, Director of Server Product Marketing at Informix,
                         about porting their Database to Linux