[Xfce4-commits] r30225 - in xfce4-appfinder/trunk: . src

[Jannis Pohlmann]

On Thu, 09 Jul 2009 00:01:56 -0700
"Brian J. Tarricone" <brian at tarricone.org> wrote:

> On 2009/07/08 23:46, Colin Leroy wrote:
> > On Thu, 9 Jul 2009 08:25:01 +0200, Nick Schermer wrote:
> >
> > Hi,
> >
> >>> +      categories_array = g_new0 (const gchar *, g_list_length
> >>> (categories) + 1); +
> >>> +      for (lp = categories, n = 0; lp != NULL; lp = lp->next,
> >>> ++n)
> >>> +        categories_array[n] = lp->data;
> >
> > gchar *tmp = NULL, *categories_string = NULL;
> >
> > for (lp = categories; lp != NULL; lp = lp->next)
> >    {
> >      tmp = g_strdup_printf("%s%s%s",
> >               categories_string ? categories_string : "",
> >               categories_string ? ", " : "",
> >               lp->data);
> >      g_free(categories_string);
> >      categories_string = tmp;
> >    }
> 
> An extra allocation for each list item?  Ick.  Better to just iterate 
> over the list twice; figure out the total strlen in the first pass, 
> allocate a buffer a single time, and then do the second pass and copy 
> the chars to the new buffer.  (Hell, the original code iterates over
> the list twice [g_list_length() is O(n)] already anyway.)
> 
> Or just mostly use the original code and do g_string_append_printf()
> for each list item.  Yah, you get more allocs that way, but GString
> does a better job than just doing it manually, usually.
> 
> The original code, isn't actually that bad, depending on how 
> g_strjoinv() is implemented.  But a single malloc() for
> categories_array is likely to be slower than a pass over the entire
> list with a strlen() on each element.

I've kept the original code with minor modifications. g_strjoinv() is
the most elegant way to join several strings using a delimiter.
Internally, it iterates over the string array twice doing a malloc only
once. It basically does what you described (iterate to get the total
buffer size needed, then copy all the strings with delimiters into the
buffer). No need to re-implement that ourselves.

Using g_strjoinv() also allows the "<b>Categories:</b> %s" string to be
translated rather than just "<b>Categories:</b>". I don't know how
important it is, but I think it's better this way.

I don't really see any reason to change that code more than I did now.
It's very simple, it's easy to understand (after all, it just builds an
array of pointers to pass to g_strjoinv) and with about 4-15
categories, do we really care so much about how often we iterate over
the list and how many memcpy/malloc calls we have?

(Side note: Most of this mail is not directed at you but rather at
 Nick who started this thread. Personall, I think the exo-open issue is
 far more important.)

  - Jannis
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://mail.xfce.org/pipermail/xfce4-dev/attachments/20090709/a7630058/attachment.pgp>