Re: [PATCH] Add gn_phonebook2vcardstr()

[Pawel Kot]

Hi.

On Fri, Jun 13, 2008 at 00:33, Bastien Nocera <address@hidden> wrote:
> But here's a patch to add a function to transform phonebook entries into
> a string, instead of just pushing it to a file descriptor.
[...]
> Comments?

Yes, a few.

First general one. I don't think we need both. Having this one we
probably could:
fprintf(f, "%s", gn_phonebook2vcardstr(entry));
So we should probably give up the old one (which seems to have unused
parameter).

+static void gn_vcard_append_printf(vcard_string *str, const char *fmt, ...)

If it's static it doesn't need to be gn_. And yes, I know other static
functions in there are prefixed with gn_.

+GNOKII_API char * gn_phonebook2vcardstr(gn_phonebook_entry *entry)
+{
+       vcard_string str;
[...]
+       return str.str;

Isn't returning pointer to a local variable a bit dangerous?

And coding style issues:
+GNOKII_API char * gn_phonebook2vcardstr(gn_phonebook_entry *entry);
should be ... char *gn_...

+       {
+               char *s = "\r\n";
+               memcpy (str->end, s, 2);
+               str->end += 2;
+               str->end[0] = '\0';
+       }

I personally dislike such blocks. IMHO they decrease readibility and
there's not much gain.

+       str.str = NULL;
+       str.end = NULL;
+       str.len = 0;

or memset(&str, 0, sizeof(str))? Don't know which is better.

take care,
pkot
-- 
Fred Allen  - "Imitation is the sincerest form of television."