GList *list = e_vcard_get_attributes (evcard);

for (list = g_list_first (list);
     list != NULL;
     list = g_list_next (list))
{
         /* Do something */
}

for (list = g_list_first (list);
     list != NULL;
     list = g_list_next (list))
{
         /* Do something else */
         g_object_unref (list->data);
}

g_free (g_list_first (list));

15 thoughts on “More GList anti-patterns”

1. I’m still amazed how you lot all love C, but then try to hack C++ features into it. The C++ way seems so much better:

   list& evcard::attributes();

   glib is an abomination.

   LikeLike

2. “but then try to hack C++ features into it”

   Linked lists, clearly a feature invented in C++…

   LikeLike

3. Missing the point, which is that C++ has references, templates, classes etc. that let you write better code. The C++ version has the following advantages:

   * Improved type safety
   * No ambiguity about who has to delete the list.
   * More succinct code (I think). e.g. “g_list_length(children);” vs “children.length();”.
   * Avoids SEGFAULTS – what happens in the above code if e_vcard_stupidly_long_function() returns NULL?

   Anyway we could argue about how I’m right all day long. Better get back to work. 🙂

   LikeLike

4. @Tim
   this is exactly what I was thinking when I saw that code.
   I mean C++0x will even have “list concepts” (foreach) which eliminates these common errors even more.

   LikeLike

5. Not being a Glib user, I’d like to know: exactly what is wrong with the original code? Is it the g_free() call, or is there something wrong with the list iteration?

   LikeLike

6. oliver: I think it’s that he’s changing the value of list and then trying to use it again. The for loops should be something like:

   for (GList* node = g_list_first(list); node != NULL; node = g_list_next(node))
   {
   }

   LikeLike

7. Pingback: rojtberg.net » Why are we using C again?

8. The problem I see is that the first loop scan every item, and when it’s over, list == NULL, so the second call to g_list_first can’t work. And even if list contained the tail, g_list_first would scan the whole list just to find the first element, which is simply stupid.

   The two loops seem to have no reason to exist, they should be merged in one single loop doing all the work, an I think calling g_list_first isn’t even necessary, as e_vcard_get_attributes must be smart enough to give you the head of the list…

   LikeLike

9. @oliver:
   A GList is just an element in the list. So you have something like:
   typedef struct GList {
   struct GList *next;
   struct GList *prev;
   gpointer data;
   } GList;

   Where next points to the next element or to NULL if you are at the end of the least. prev points to the previous element or NULL if it is already the first element.
   So just NULL means an empty list.
   This is basically the standard way to implement lists, of course high level languages hide this well.

   To iterate a list you just do
   for (l = list; l != NULL; l = l->next)
   {
   }

   So at the end of the iteration l will be NULL (i.e. empty list) and calling g_list_first on NULL returns NULL.

   LikeLike

10. you have to admire the power of c++, if c++ developers have all the free time to go on random blogs over the web in order to write comments completely missing the point about algorithmic mistakes that are completely language-independant.

    either that, or c++ really don’t have anything better to do.

    LikeLike

11. Sorry but it’s *not* language independent. For example if I try to mess up the C++ API as badly as the C one I get this:

    list<anobject>& lst = evcard.attributes();

    for (lst = lst.begin(); lst != lst.end(); ++lst) // doesn’t compile
    {
    }

    delete lst; // doesn’t compile

    This clearly has no chance of compiling, unlike the C version.

    LikeLike

12. Whoever wrote this would find a way to make a disaster out of it in C++ just as much as in C, I’m sure.

    I don’t know much about using GLib (but I do know how a linked list works!), but on top of what Marco said was wrong about the iteration itself (which you can screw up in C++ in a few ways, most easily by modifying the list itself while iterating, if you’re not careful), imagining that he moved the unref to the first list or correctly kept a copy of the head pointer, he would be leaving behind what would look like a populated list, full of dangling pointers to memory that has (probably, it’s a refcount, after all) been freed. If he’s going to delete the items, he should also delete the list nodes.

    Most of all, he doesn’t understand ownership correctly, not having bothered to check who’s the owner of e_vcard_get_attributes()’s return value. There’s an advantage to C++ here in theory, since in C you have to check the documentation, or the code of that function in order to know whether you take ownership or not, but in C++ you can codify it with smart pointers, and figure it out simply looking at the function signature. But this guy, if he wrote a function like e_vcard_get_attributes(), he’d probably used shared_ptr everywhere, using the “big hammer that works most of the time (but gives the nearly impossible to debug memory leaks sometimes, oh well)”, because he simply doesn’t understand ownership.

    C++ and fancy smart pointers don’t free you from having to understand what’s going on, it just helps with the tedium of dealing with the details. You can just say “this object should be owned by a single owner at all time” by using a scoped_ptr, and let the compiler enforce this.

    You still have to know you want this in the first place, though!

    LikeLike

13. If you wanna mess it up in C++ (which you cannot as easily in C), you’d write:
    list lst = evcard.attributes();
    And then you’d get a g_list_copy().

    But Marco, what went wrong about that code? The second loop doesn’t run, g_free (NULL) is fine too. So it should just work (even though it looks very bad).

    LikeLike

14. @Benjamin:
    Marco said that it didn’t work, not that it crashed…

    The developer wanted the second loop to use the data items, but is never executed.

    Moreover, if the second loop was corrected, the fact that the developer wants to free the items and the list would lead to a crash…

    So it’s just not the developer wanted to do, which was it seems something like this :

    GList *list = e_vcard_get_attributes (evcard);
    GList *l;
    for (l = list; l != NULL; l = l->next)
    {
    /* Do something */
    /* Do something else */
    }

    LikeLike

15. Oh yeah, I missed the “do something else” part. I thought the second loop just unreffed the items.

    LikeLike

Comments are closed.