Re: Patch with lots of cleanups for warnings from sparse and the Intel compiler

[Rodrigo Moya <rodrigo novell com>]

On Thu, 2005-11-10 at 18:52 +0100, Kjartan Maraas wrote:
> tir, 08,.11.2005 kl. 12.58 +0100, skrev Rodrigo Moya:
> > On Tue, 2005-11-01 at 22:52 +0100, Kjartan Maraas wrote:
> > > This does the following:
> > > 
> > > - mark code static when possible
> > > - remove unused code
> > > - make some parts ANSI compliant
> > > - fix some format specifiers
> > > 
> > some comments:
> > 
> > > Index: capplets/keybindings/gnome-keybinding-properties.c
> > > @@ -442,7 +442,6 @@ should_show_key (const KeyListEntry *ent
> > >        return TRUE;
> > >      else
> > >        return FALSE;
> > > -    break;
> > >    }
> > this looks ok, although it might make older gcc's to show warnings about
> > it, AFAIK.
> > 
> Do you know how old? Do they compile GNOME at all?
> 
AFAIR, gcc 3.3.x, but not sure, so let's just ignore it for now.

> > > @@ -600,15 +601,15 @@ gnome_theme_installer_run (GtkWidget *pa
> > >         running_theme_install = TRUE;
> > >  
> > >         if (!g_file_test ("/bin/tar", G_FILE_TEST_EXISTS)) {
> > > -               GtkWidget *dialog;
> > > +               GtkWidget *error_dialog;
> > >  
> > > -               dialog = gtk_message_dialog_new (NULL,
> > > +               error_dialog = gtk_message_dialog_new (NULL,
> > >                                                 GTK_DIALOG_MODAL,
> > >                                                 GTK_MESSAGE_ERROR,
> > >                                                 GTK_BUTTONS_OK,
> > >                                                 _("Cannot install
> > > theme.\nThe tar program is not installed on your system."));
> > > -               gtk_dialog_run (GTK_DIALOG (dialog));
> > > -               gtk_widget_destroy (dialog);
> > > +               gtk_dialog_run (GTK_DIALOG (error_dialog));
> > > +               gtk_widget_destroy (error_dialog);
> > >                 return;
> > >         }
> > here, why don't you declare one GtkDialog just once, as you were doing
> > in your previous patch?
> > 
> Have gone back to declaring it only once in tranfer_done_cb(). 
> 
> I did this in gnome_theme_installer_run() because the first declaration
> is GladeXML *dialog; and the latter is assuming a GtkDialog * and I
> don't think they are interchangeable?
> 
no, they are not interchangeable.

> > > @@ -115,6 +115,5 @@ AcmeVolume *acme_volume_new (void)
> > >         vol = ACME_VOLUME  (g_object_new (acme_volume_oss_get_type (),
> > > NULL));
> > >         return vol;
> > >  #endif
> > > -       return ACME_VOLUME  (g_object_new (acme_volume_dummy_get_type
> > > (), NULL));
> > >  }
> > AFAICS, there are code patchs that can reach this last line, so why not
> > just return NULL there instead of removing the return statement?
> > 
> The compiler said this was unreachable though. Are you sure it is wrong?
> Do you mean the case where neither of the #ifdef's are true? Why not
> just return NULL in that case if it's something we care about?
> 
right, if it's reachable, a 'return NULL' there would be good. If the
compiler says it's unreachable, just ignore me.

> > The rest looks ok, provided it compiles, so please answer back on these
> > comments, and once we're set, I think this can be committed to both HEAD
> > and the gnome-2-12 branch
> 
> New patch attached.
> 
ok, this one looks good, so please commit to both HEAD and the
gnome-2-12 branch.
-- 
Rodrigo Moya <rodrigo novell com>