On Mon, 2014-04-07 at 19:59 +0600, Alexander E. Patrakov wrote:
> 07.04.2014 18:24, Tanu Kaskinen wrote:
> >
> > +/* Called from the main thread. */
> > +static void set_reference_volume_internal(pa_sink *s, const pa_cvolume *volume) {
> > + pa_cvolume old_volume;
> > + char old_volume_str[PA_CVOLUME_SNPRINT_VERBOSE_MAX];
> > + char new_volume_str[PA_CVOLUME_SNPRINT_VERBOSE_MAX];
> > +
> > + pa_assert(s);
> > + pa_assert(volume);
>
> Note the assertions.
>
> > +
> > +/* Called from the main thread. */
> > +void pa_sink_set_reference_volume_direct(pa_sink *s, const pa_cvolume *volume) {
> > + pa_assert(s);
> > + pa_assert(volume);
> > +
> > + set_reference_volume_internal(s, volume);
> > +}
>
> What's the point of having this function in this patch? As demonstrated
> above, set_reference_volume_internal does the same assertions.
The "point" is in the function naming: I like to have a function called
set_reference_volume_internal(), but it makes no sense to have a
function with that name exported outside the .c file, so I added another
function for interfacing with the outside world. I can't give good
reasons for why I like the name set_reference_volume_internal(), so I
think it's best that I drop set_reference_volume_internal() and move the
implementation inside pa_sink_set_reference_volume_direct().
> Same for sources.
>
> The patch does look like an equivalent refactoring.
Thanks for the review!
--
Tanu
