[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Add gn_phonebook2vcardstr()

From: Pawel Kot
Subject: Re: [PATCH] Add gn_phonebook2vcardstr()
Date: Fri, 13 Jun 2008 20:47:00 +0200


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

+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,
Fred Allen  - "Imitation is the sincerest form of television."

reply via email to

[Prev in Thread] Current Thread [Next in Thread]