[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 |
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."