libspeechd SPDVoice cleanup api

[Jeremy Whiting]

Trevor,

Thanks for the review. I've attached an updated patch and will answer inline.

On Thu, Oct 23, 2014 at 11:52 PM, Trevor Saunders <tbsaunde at tbsaunde.org> 
wrote:
> On Thu, Oct 23, 2014 at 11:29:28PM -0600, Jeremy Whiting wrote:
>> Hey all,
>>
>> The current libspeechd has api to get a list of voices for the
>> synthesizer, but no api to safely clean up the data. The attached
>> patch adds a new method free_spd_voices to do that. It iterates over
>> the SPDVoice ** list freeing the strdup'ed strings and the SPDVoice
>> structures themselves. I tested this with a modified qtspeech to use
>> this new api and it builds and runs fine here. Let me know if I need
>> to modify it in any way.
>
> makes sense.
>
>>
>> thanks,
>> Jeremy
>>
>> P.S. is there some patch review tool available from brailcom or
>> something I should be using instead?
>
> not that I'm aware of and personally I tend to prefer reviewing in
> email.
>
>
>> From 999f0280b1a0ac075e15296235b4b38fe55eb613 Mon Sep 17 00:00:00 2001
>> From: Jeremy Whiting <jpwhiting at kde.org>
>> Date: Thu, 23 Oct 2014 23:24:30 -0600
>> Subject: [PATCH] Add free_spd_voices convenience method to free SPDVoice
>>  structures.
>>
>> Iterates over SPDVoice list and frees strings and SPDVoice structures.
>> ---
>>  src/api/c/libspeechd.c | 10 ++++++++++
>>  src/api/c/libspeechd.h |  1 +
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/src/api/c/libspeechd.c b/src/api/c/libspeechd.c
>> index 3f693b0..b9b918a 100644
>> --- a/src/api/c/libspeechd.c
>> +++ b/src/api/c/libspeechd.c
>> @@ -1221,6 +1221,16 @@ SPDVoice **spd_list_synthesis_voices(SPDConnection * 
>> connection)
>>       return svoices;
>>  }
>>
>> +void free_spd_voices(SPDVoice** voices)
>
> super picky, but wouldn't SPDVoice **voices be more consistant?
>
>> +{
>> +     int i = 0;
>> +     while (voices != NULL && voices[i] != NULL) {
>
> I'd say this function probably shouldn't accept a null voice list, or at
> least the check should be manually hoisted out of the loop.

Yep, removed the null check.

>
>> +             free(voices[i]->name);
>> +             free(voices[i]);
>> +             ++i;
>> +     }
>
> you could get rid of the extra variable and do
>
> for (; *voices; voices++) {
>   free((*voices)->name);
>   free(*voices);
> }

If I do this I wont be able to free voices afterwards. So I left that as is.

>
>> +}
>
> and shouldn't you free the array of pointers?

Done.

>
> Thanks!
>
> Trev
>
>> +
>>  char **spd_execute_command_with_list_reply(SPDConnection * connection,
>>                                          char *command)
>>  {
>> diff --git a/src/api/c/libspeechd.h b/src/api/c/libspeechd.h
>> index be0d439..4d5c047 100644
>> --- a/src/api/c/libspeechd.h
>> +++ b/src/api/c/libspeechd.h
>> @@ -221,6 +221,7 @@ int spd_get_message_list_fd(SPDConnection * connection, 
>> int target,
>>  char **spd_list_modules(SPDConnection * connection);
>>  char **spd_list_voices(SPDConnection * connection);
>>  SPDVoice **spd_list_synthesis_voices(SPDConnection * connection);
>> +void free_spd_voices(SPDVoice** voices);
>>  char **spd_execute_command_with_list_reply(SPDConnection * connection,
>>                                          char *command);
>>
>> --
>> 2.1.2
>>
>
>> _______________________________________________
>> Speechd mailing list
>> Speechd at lists.freebsoft.org
>> http://lists.freebsoft.org/mailman/listinfo/speechd
>
>
> _______________________________________________
> Speechd mailing list
> Speechd at lists.freebsoft.org
> http://lists.freebsoft.org/mailman/listinfo/speechd
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-free_spd_voices-convenience-method-to-free-SPDVo.patch
Type: text/x-patch
Size: 1508 bytes
Desc: not available
URL: 
<http://lists.freebsoft.org/pipermail/speechd/attachments/20141024/9ededd7f/attachment-0001.bin>