Created attachment 59995 [details] [review]
Patch

Created attachment 59996 [details] [review]
Fixes an issue in the prev one

A probably more useful function is one that takes a GString and fills it in.

A GString still uses mallocs. I do not need to build a string at all, just inspect a string unichar by unichar.

Created attachment 60471 [details]
Testcase

A testcase for the patch.

If not anything else, the patch should avoid such code duplication.  Make both functions call another one, with a flag to malloc or not.

Created attachment 95725 [details] [review]
Patch with requested changes

Should not use 'malloc' as variable name.  I'll see if I can cleanup and commit this this week.

Instead of promising and not delivering, I'll promise to commit this if a finished patch is submitted :).

Created attachment 147259 [details] [review]
Rename 'malloc' variable to 'do_malloc'.

Needs to add new symbol to list of symbols and docs, but I can do that part.

Matthias, does this look good to go?

Not too fond of the real_g_ prefix. I'd prefer something like do_canconical_decomposition or decompose_canonically, but not a biggie. I'll let you pick...

And I wonder if we really need the separate do_malloc parameter. 
Can't out == NULL used to signal this ?


Other nitpicks: 

+ * @result_len: location to store the length of the return value.

Would be good to add a clarifying 'in bytes' - if it is in bytes after all. 
This line makes me think it is not:

-      r = g_malloc (*result_len * sizeof (gunichar));


Other than that, looks ok

Yeah, I'll do a cleanup/review before committing.  I was more interested in your opinion re the function name.

Created attachment 155313 [details] [review]
0001-Minor-fixes-to-new-g_unicode_canonical_decomposition.patch

From b12038fdea2da56795917a7b24879fb5637dd779 Mon Sep 17 00:00:00 2001
From: Claudio Saavedra <csaavedra@igalia.com>
Date: Fri, 5 Mar 2010 16:15:23 +0200
Subject: [PATCH] Minor fixes to new g_unicode_canonical_decomposition_with_buffer()

Add it to the documentation, set its "Since:" mark, rename internal
method, get rid of unneeded parameter, minor documentation fixes,
and fix indentation.
---
 docs/reference/glib/glib-sections.txt |    1 +
 glib/gunidecomp.c                     |   61 ++++++++++++++++-----------------
 2 files changed, 31 insertions(+), 31 deletions(-)

Above patch addresses concerns raised here and also fixes indentation and minor doc. issues.

(In reply to comment #12)
> Would be good to add a clarifying 'in bytes' - if it is in bytes after all. 
> This line makes me think it is not:
> 
> -      r = g_malloc (*result_len * sizeof (gunichar));

It's not in bytes, as far as I can tell.

I'd be glad to commit, since we have this patch in maemo and I'd like to get rid of it.

(In reply to comment #15)
> Above patch addresses concerns raised here and also fixes indentation and minor
> doc. issues.

Thanks.

> (In reply to comment #12)
> > Would be good to add a clarifying 'in bytes' - if it is in bytes after all. 
> > This line makes me think it is not:
> > 
> > -      r = g_malloc (*result_len * sizeof (gunichar));
> 
> It's not in bytes, as far as I can tell.

Then lets make it clearn in the docs that it's number of characters.

> I'd be glad to commit, since we have this patch in maemo and I'd like to get
> rid of it.

Fine with me.  Maybe while at it add a couple unit tests too?

Matthias?

Going through some of my old patches.. does this make any sense or can I pull the plug on it? I won't have time to add unit tests unfortunately tho.

Oh I actually independently added something like this.  See:

gsize g_unichar_fully_decompose (gunichar  ch, 
                                 gboolean  compat, 
                                 gunichar *result, 
                                 gsize     result_len);