Re: GNOME CVS: glib sopwith

[Owen Taylor <otaylor redhat com>]

gnomecvs cvs gnome org (Gnome CVS User) writes:

> CVSROOT:	/cvs/gnome
> Module name:	glib
> Changes by:	sopwith	00/11/28 18:44:21
> 
> Modified files:
> 	.              : ChangeLog gcompletion.c gcompletion.h gmarkup.c 

ChangeLog entry for gmarkup.c? and speaking of that:

  /* Called for open tags <foo bar="baz"> */
  void (*start_element)  (GMarkupParseContext *context,
                          const gchar         *element_name,
                          const gchar        **attribute_names,
                          const gchar        **attribute_values,
                          gpointer             user_data,
                          GError             **error);

====
                  if (context->parser->start_element)
                     (* context->parser->start_element) (context,
                                                         start_name,
-                                                        attr_names,
-                                                        attr_values,
+                                                        (const gchar *)attr_names,
+                                                        (const gchar *)attr_values,
                                                         context->user_data,
                                                         &tmp_error);

====

Tell me if you see something wrong here... (if you don't the
compiler certainly does)

> 	                 gunidecomp.c guniprop.c 
> 	gobject        : ChangeLog glib-genmarshal.c gsignal.c 
> 	tests          : shell-test.c 

[...]
> 
> * gcompletion.[ch]: Add g_completion_set_compare(),
> to allow (for example) using case-insensitive completion.

API docs?

+typedef int (*GCompletionStrcmpFunc)(const char *s1, const char *s2);
+typedef int (*GCompletionStrncmpFunc)(const char *s1, const char *s2, size_t n)

Why two typedefs when you only use the second?

+void
+g_completion_set_compare(GCompletion *cmp,
+                        GCompletionStrncmpFunc strncmp_func)

<broken_record>Please align your parameter names</broken record>
 
> * gobject/gsignal.c: Fix warnings about possible use of uninitialized
> variables, and fix logic that would leave 'node' unset in cases
> that it might be used in.

     {
       HandlerList *hlist = handler_list_lookup (signal_id, instance);
       Handler *handler;
-      SignalNode *node;
+      SignalNode *node = NULL;

It's best here to indicate that the assignment is bogus - I
use:

+      SignalNode *node = NULL;          /* Quiet gcc */

=====
-      if (mask & G_SIGNAL_MATCH_FUNC)
+      if (!(mask & G_SIGNAL_MATCH_FUNC))
        {
          node = LOOKUP_SIGNAL_NODE (signal_id);
          if (!node || !node->c_marshaller)
=====

Let see, we look up the node containing the function only if we
_aren't_ matching against the function... Please look over
the logic again.

(Yes, this is highly confusing code, since there is a mask = ~mask
slightly farther down, and in the next block, the mask = ~mask occurs
_before_ the corresponding check.

The conclusion here is it is best to send your patches to Tim
first...)   
 
> * gobject/glib-genmarshal.c: Fix warning about printf format.

-      g_warning ("overfull string (%u bytes) for padspace", strlen (string));
+      g_warning ("overfull string (%lu bytes) for padspace", strlen (string));
 
No guarantee that size_t is the same as long either... the only C89 way
of doing this is:

+      g_warning ("overfull string (%lu bytes) for padspace", (gulong)strlen (string));

I think C99 might have a format character for size_t.