Re: [PATCH] Add gn_phonebook2vcardstr()

[Bastien Nocera]

On Fri, 2008-06-13 at 20:47 +0200, Pawel Kot wrote:
> 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

I didn't want to break ABI compat.

>  (which seems to have unused
> parameter).

Yes, I noticed that as well. I guess the location is now part of the
entry struct itself.

> +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_.

:) I prefer prefixing, but I don't really mind if functions names are
changed.

> +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?

str.str is allocated on the heap, only str is on the stack.

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

Save allocations, but I'll fix that up.

> +     str.str = NULL;
> +     str.end = NULL;
> +     str.len = 0;
> 
> or memset(&str, 0, sizeof(str))? Don't know which is better.

My mistake, I changed some bits around there, and didn't "optimise"
after the fact. Will fix.

The main question is:
- Do you want to keep the old function
- What name do you want it to have

Cheers