Ximian Hackers Self Help: Part 1

Novell Cool Solutions: Trench

Digg This - Slashdot This Posted: 14 Jul 2004

These self-help letters began as a post to an internal Ximian development list. We've posted them here in a three-part series to provide you with insight into the development process at Ximian and within the GNOME community at large.

• Part 1
• Part 2
• Part 3

---

Contents

Fatal Instinct: Bad List Iteration

A Good Pattern: Good List Iteration

A Lesson in Taste

Did You Know?

	Fatal Instinct: Bad List Iteration

[ broken ]
   GSList *l;

   for (l = master; l; l = l->next) {

      if (matches (l->data, filter))

      master = g_slist_remove (
                  master, l->data);
   }

So ... the g_slist_remove modifies the list under the loop walking the list, often the 'remove' is less obvious, but this is always broken, a better way to do it is this:

for (l = priv->listeners; l; l = next) {
      ListenerDesc *desc = l->data;

      next = l->next;

      if (desc->id == id) {
         priv->listeners = g_list_remove_link (
            priv->listeners, l);
         g_list_free_1 (l);
         desc_free (desc, ev);
         return;
      }
   }

Not only does this use the improved, and clearer 'next' variable to move onto the next list item, but it also uses g_list_remove_link, which removes the node without scanning the whole list for elements matching that data pointer.

	A Good Pattern: Good List Iteration

When iterating lists, there is no point in using the g_list_first / next () macros; as so much code uses the direct structure names, it is not possible to change them, and they only clutter the code. I also find the "for ()" idiom very clear, in contrast to the while idiom:

Nice.                                 Nasty.
 ----                                 -----

GSList *l                             GSList *l

for (l = start; l; l = l->next) {     l = start;
   ...                                while (l) {
}                                             ...
                                              l = l->next;
                                      }

Not only is the latter longer, but the full body of the loop needs to be inspected to ascertain what exactly is going on, and clearly 'continue' cannot be used to nice effect.

	A Lesson in Taste

By extensive experimentation, and careful measuring of code quality across millions of lines of code, it has been conclusively discovered that 2, 3 or 4 stop indents are the devil's tool. Tabs are 8 stops, so are indents.

See: /usr/doc/linux/Documentation/CodingStyle - chapter 1.

	Did You Know?

In both glib 1 and 2, g_hash_table insert does a strange thing, so - people who don't like leaks do things like this when they insert into a table.

Broken:

void
my_insert_fn (GHashTable *ht, const char *foo, gpointer value)
{
   gpointer old_key;
   gpointer old_value;

   if (g_hash_table_lookup_extended (ht, foo, &old_key, &old_value))
      g_free (old_key);

   g_hash_table_insert (ht, g_strdup (foo), value);
}

Sadly, this code is broken, since when a duplicate node is inserted into a hash table it is the old key is retained, and the new key is not inserted. For obscure reasons, this is a feature.

There are several ways to fix the code, one of the easiest is:

   void
   my_insert_fn (GHashTable *ht, const char *foo, gpointer value)
   {
      gpointer old_key;
      gpointer old_value;

      if (g_hash_table_lookup_extended (ht, foo, &old_key,
&old_value)) {
+         g_hash_table_remove (ht, foo);
+         g_free (old_key);
+      }

       g_hash_table_insert (ht, g_strdup (foo), value);
   }

Note: All of the above errors as seen in real code written by real people.

Reader Comments

• Author, please read GList , GSlist and GHashTable docs before writing such ...

Novell Cool Solutions (corporate web communities) are produced by WebWise Solutions. www.webwiseone.com