Re: [PATCH] Add gn_phonebook2vcardstr()

[Pawel Kot]

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?

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

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

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