Re: gtk+-0.99.5 bug report

[Owen Taylor <owt1 cornell edu>]

"Arthur Hagen" <art@broomstick.com> writes:

> Here are some of the bugs I found in gtk+-0.99.5 in digest format

OK, first a comment.

Specifying the bugs by line numbers doesn't work because the
line numbers in the current version of GTK are just a bit different
than those in 0.99.5. 

Please give the relevant fragments of code, or send unified diffs (or
context diffs if you don't have GNU diff) in the future. Thanks!

[ Also, it would be nice to say what system and compiler you are
  using, so we can remember it for future reference when we 
  want something really picky. ;-) ]
 
> ----------
> 
> Subject: bug #1: gdkwindow.c, lines 65 and 80; lines 113 and 123.
> 
> i is an declared an unsigned int in line 65 and 113, but is tested in
> line 80 and 123 for >= 0, which is *always* true for unsigned variables.
> Only a bug in gcc allows this to work.
> 
> Fix:
> 
> Declare i as an int.

It doesn't work even on gcc. It was quickly fixed.
 
> Subject: bug #2: gdkwindow.c, lines 1724 and 1735.
> 
> wm_hints is declared as a local stack variable but not initialized to zero.
> Using |= in line 1735 makes no sense, because the value can be anything
> before the or is done.
> 
> Fix:
> 
> Use "bzero(&wm_hints,sizeof(wm_hints));" before the or.

Either this has been fixed, or I don't understand your 
problem.

  XWMHints wm_hints;

  [...]

  wm_hints.flags = 0;
  
  if (icon_window != NULL)
    {
      private = (GdkWindowPrivate *)icon_window;
      wm_hints.flags |= IconWindowHint;
      wm_hints.icon_window = private->xwindow;
    }

  etc.

The flags field is initialized. Other fields are only filled in
if the corresponding field in flags is set. If you compiler
complains about this, it is broken. :-)

> ----------
> 
> Subject: bug #3: gtk/Makefile and gdk/Makefile
> 
> The makefiles has dependencies that won't work with most versions of
> make. "$(top_builddir)/gtk/libgtk.la" resolves to "../gtk/libgtk.la"
> for which there is no makefile rule.  There is one for "libgtk.la",
> but make on most systems isn't smart enough to handle path resolving
> on the left hand side of a dependancy.
> 
> Fix:
> 
> Remove "$(top_builddir)/gtk" and "../gtk/" everywhere in gtk/Makefile,
> and "$(top_builddir)/gdk" in gdk/Makefile.

I can't find these in my Makefiles. (e.g., the rule for libgtk.la reads,

libgtk.la: $(libgtk_la_OBJECTS) $(libgtk_la_DEPENDENCIES)
	$(LINK) -rpath $(libdir) $(libgtk_la_LDFLAGS) $(libgtk_la_OBJECTS) $(libgtk_la_LIBADD) $(LIBS)

Did you, by any chance, run automake on the package? Which version?

This would be a automake bug <automake-bugs@gnu.ai.mit.edu>, if
anything. But I don't see the bug, either with automake 1.2d or
1.2f. (I'm not sure what the distribution was built with though)

> Subject: bug #4: gtkclist.h, lines 181, 195 and 196
> 
> Also:
> gtkhandlebox.h, lines 53, 54 and 55
> gtktooltips.h, lines 61 and 62
> 
> gint is signed, and a bitfield of width 1 is declared.
> A bitfield of width 1 will not have room for the sign bit, and will
> become negative just by being set.

This isn't really a bug, because these are used as booleans, and
-1 is just as true as 1 (as long as nobody does if (foo.x == TRUE)). 
But the GTK standard is to make such bitfields unsigned. So I've
changed gtkhandlebox.h

> ----------
> 
> Subject: bug #5, gtkclist.c, lines 1401 and 1405
> 
> Typecasting the lvalue in assignments is not permitted under ANSI C,
> and will give errors when compiling with anything stricter than gcc.
> 
> Fix:
> 
> Change the (gint) typecast on the right hand side, and remove the
> left hand side typecast:
> 
> list->data = (gpointer)((gint) list->data + 1);
> 
> list->data = (gpointer)((gint) list->data - 1);

Some unknown person has changed it so that it is now:

          case SYNC_INSERT:
            list->data = ((gchar*) list->data) + 1;
            break;

          case SYNC_REMOVE:
            list->data = ((gchar*) list->data) - 1;
            break;

which is an ugly hack, and should be (gchar8*), but fixes the
immediate problem.
> 
> Note that this assumes that pointers are the same size as ints, which
> is a *bad* *bad* *bad* assumption.

No, it merely assumes pointers are _as big as_ ints. Which is
a very good assumption on anything that GTK is at all likely
to be compiled on.

But, to avoid complaints from compilers, such casts should be
done as

  (gpointer)(glong)some_int

> Subject: bug #6: gtkentry.c, line 1582; gtkeditable.h, line 45
> 
> editable->current_pos is declared unsigned, but checked whether it is
> negative.
> 
> Fix:
> 
> Change current_pos in gtkeditable.h from guint to gint.

Well, there is a good case to be made that all guint's
should be banned from the gtkeditable/entry/text code, but barring
that, a safe fix is simply to remove the offending comparison.
Which I've done locally.

> ----------
> 
> Subject: bug #7: gtktext.c, lines 606 and 610
> 
> index is declared as unsigned in line 606, but compared for >= 0 in
> line 610.
> 
> Fix:
> 
> Remove pointless comparision from line 610

Yep.

> ----------
> 
> Subject: bug #8: gtktext.c, lines 825 and 832
> 
> The pointer text is allocated from the stack in line 825, but not given
> a value before being taken the length of in line 832.
> 
> Fix:
> 
> Move the test for end_pos < 0 down a few lines to after text is set.

Already fixed.

Thanks for the report,
                                        Owen