Re: [PATCH] Add gn_phonebook2vcardstr()

[Bastien Nocera]

On Sun, 2008-06-15 at 21:43 +0200, Pawel Kot wrote: 
> Hi,
> 
> On Fri, Jun 13, 2008 at 23:32, Bastien Nocera <address@hidden> wrote:
> > 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.
> 
> Fair enough. However could you please mark it to be removed in the
> next libgnokii major version?

Done.

> >> +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.
> 
> The original idea was to prefix just public functions so they were
> easily recognized.

Done.

> >> +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.
> 
> OK.
> 
> > The main question is:
> > - Do you want to keep the old function
> 
> See above.
> 
> > - What name do you want it to have
> 
> Current one seems fine.

Cool. Committed.