libspeechd SPDVoice cleanup api

[Trevor Saunders]

On Fri, Oct 24, 2014 at 02:08:33PM -0600, Jeremy Whiting wrote:
> 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?

I see you left that, but I'm not going to hasle you about it given this
code seems pretty inconsistant.

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

yeah, you caught me ;) lgtm now fwiw.

Trev

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

> From 1112091cbdd8467b314918b4070e2e467bbcf87f 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 | 11 +++++++++++
>  src/api/c/libspeechd.h |  1 +
>  2 files changed, 12 insertions(+)
> 
> diff --git a/src/api/c/libspeechd.c b/src/api/c/libspeechd.c
> index 3f693b0..a8eb409 100644
> --- a/src/api/c/libspeechd.c
> +++ b/src/api/c/libspeechd.c
> @@ -1221,6 +1221,17 @@ SPDVoice **spd_list_synthesis_voices(SPDConnection * 
> connection)
>       return svoices;
>  }
>  
> +void free_spd_voices(SPDVoice** voices)
> +{
> +     int i = 0;
> +     while (voices[i] != NULL) {
> +             free(voices[i]->name);
> +             free(voices[i]);
> +             ++i;
> +     }
> +     free(voices);
> +}
> +
>  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..f55bc97 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