[Freeciv-Dev] Re: a plenty of bugs (PR#1571)

[Mike Kaufman <kaufman@xxxxxxxxxxxxxxxxxxxxxx>]

yikes. a lot of your bugs have been fixed. you should have reexamined
the code post-1.11.5.

On Sat, Jun 15, 2002 at 02:59:54AM -0700, sylvaintricot@xxxxxxx wrote:
> /*************************************************************************/
> unused id "j" in /freeciv-cvs-Mar-27/client/gui-gtk/gotodlg.c
> 
> 198:
>     for(i=0, j=0; i<game.nplayers; i++) {

yep.

> 
> /*************************************************************************/
> unused code is obsolete in /freeciv-cvs-Mar-27/ai/aicity.c
> 
> 443:
>     ai_city_defender_value(pcity, U_LEGION, bestchoice.choice)) * 2 &&
> 
> U_LEGION is no longer defined.

yes, GRAVEDANGERWORKS code will probably be wasted as ai code is
overhauled.

> 
> /*************************************************************************/
> 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?

good question. how do you know it's deliberate?

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

ok.

> 
> /*************************************************************************/
> bug in /freeciv-cvs-Mar-27/client/xaw/diplodlg.c
> 
> function:
> struct Diplomacy_dialog *create_diplomacy_dialog(struct player *plr0,
>                                                struct player *plr1)
> 
> 533:
>         get_ruler_title(plr1->government, plr0->is_male, plr0->nation),
> 
> correction:
>         get_ruler_title(plr1->government, plr1->is_male, plr1->nation),

indeed.

> 
> /*************************************************************************/
> suspicious code in /freeciv-cvs-Mar-27/server/mapgen.c
> 
> 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

I've asked Ross to look at these mapgen changes.

> 
> /*************************************************************************/
> 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 has been changed in CVS

> 
> /*************************************************************************/
> unused definition in
> /home/styx/st/dev/fc/freeciv-cvs-Mar-27/client/include/colors_g.h
> 
> 16:#define MAX_COLORS 256

yep.

> 
> /*************************************************************************/
> unused declaration in /freeciv-cvs-Mar-27/client/gui-xaw/colors.h
> 
> 22:int get_colors(Display *display);

yep.

> 
> /*************************************************************************/
> line missing in /freeciv-cvs-Mar-27/client/gui-win32/colors.c
> line missing in /freeciv-cvs-Mar-27/client/gui-mui/colors.c
> 
> 24:  { 86,  86,  86},  /* Background (gray) */

mui exists. win32 doesn't. I don't know whether this is purposeful or
not. Andreas?

> 
> /*************************************************************************/
> 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?:
> ???

there seems to be a couple places where this is still the case. I can't
speak to it. There have been a lot of changes in here since 1.11.5
Perhaps Jason can speak to this?

> 
> /*************************************************************************/
> 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);

map_adjust is pretty much gone everywhere in CVS.

> 
> /*************************************************************************/
> 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);

this appears ok.

> 
> /*************************************************************************/
> 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);

fixed in CVS

> 
> /*************************************************************************/
> 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 */

this code no longer exists.

> 
> /*************************************************************************/
> 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;

this is now on 2146
hmm. are you sure?

      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;
      } else {
        pl->goals.tech[j] = A_NONE;
      }

perhaps?

The question is whether it's bad for goals.tech to be A_LAST...
the only use is ai/aitech.c:102. It's not quite clear. Syela mentions
that this is basically not used anyway... A question for Raimar.


> 
> /*************************************************************************/
> 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 )

I tend to agree, but Raimar (via Per) put a bunch more of this type (omitting
unused parameters in the function) in the code recently.

> 
> 
> and so on.
> 
> PS: it's OK but bad code :8(
> 
> /*************************************************************************/
> 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)) {

this is fixed in CVS

-mike