[pulseaudio-discuss] [PATCH RFCv3 43/51] core: Add volume-util.h

[Arun Raghavan]

On 10 November 2014 15:15, Peter Meerwald <pmeerw at pmeerw.net> wrote:
> Hallo,
>
>> >>>> implement inlineable functions PA_CVOLUME_VALID(),
>> >>>> PA_CVOLUME_CHANNELS_EQUALS_TO(), PA_CVOLUME_IS_MUTED(),
>> >>>> PA_CVOLUME_IS_NORM() that assume data is valid
>> >>>
>> >>> Why are these uppercase? AFAIK, we don't usually uppercase inline
>> functions.
>> >>
>> >> I've modelled after e.g. PA_ALIGN_PTR() in pulsecore/macro.h or
>> >> PA_SOURCE_IS_LINKED() in source.h
>> >>
>> >> I think there is no clear rule; I'd prefer PA_CVOLUME_IS_NORM() to
>> >> pa_cvolume_is_norm_internal() or pa_cvolume_is_norm_unchecked()
>> >>
>> >> thanks, p.
>> >
>> > My preference is lower case for functions (I suspect the exceptions
>> > began as macros).
>>
>> My preference is to avoid uppercase too. I think it's okay to move the
>> pa_assert to pa_assert_fp in general if the asserts end up being CPU
>> intensive - rather than making one pa_cvolume_is_norm() and one
>> pa_cvolume_is_norm_unchecked() function.
>
> there is the issue of
> * inlineability (function under pulse/ are API/ABI)
> * avoiding checks with NDEBUG
> * naming (I'm fine with lowercase)
>
> I suggest to add new functions under pulsecore/ that mirror those under
> pulse/ but are inlineable and do less checking
>
> let's take pa_cvolume_channels_equal_to() in pulse/volume.c as an example:
>
> int pa_cvolume_channels_equal_to(const pa_cvolume *a, pa_volume_t v) {
>     unsigned c;
>     pa_assert(a);
>
>     pa_return_val_if_fail(pa_cvolume_valid(a), 0);
>     pa_return_val_if_fail(PA_VOLUME_IS_VALID(v), 0);
>
>     for (c = 0; c < a->channels; c++)
>         if (a->values[c] != v)
>             return 0;
>
>     return 1;
> }
>
> the function is called often in critial paths, non-inlineable and the
> checks don't go away with NDEBUG since they use pa_return_val_if_fail()
> and are mandated by the API
>
> so I'd suggest to add a function pa_cvolume_channels_equal_to_internal()
> in pulsecore/volume-util.h:
>
> static inline int pa_cvolume_channels_equal_to_internal(const pa_cvolume
> *a, pa_volume_t v) {
>     unsigned c;
>     pa_assert_fp(a);
>
>     pa_assert_fp(pa_cvolume_valid(a), 0);
>     pa_assert_fp(PA_VOLUME_IS_VALID(v), 0);
>
>     for (c = 0; c < a->channels; c++)
>         if (a->values[c] != v)
>             return 0;
>
>     return 1;
> }
>
> this results in
> (1) the name is rather long and ugly, yet lowercase
> (2) the function is inlineable and not part of the API/ABI
> (3) checks will be go away with OPTIMIZE or NDEBUG, the behaviour is
> DIFFERENT compared to pa_cvolume_channels_equal_to() -- the
> assumption is that cvolumes and volumes are always valid internally

This sounds reasonable to me. If you want to make the name slightly
shorter, we could use _fast() as the suffix (if not, I think
_unchecked() as David suggested makes sense, as it makes the semantics
clearer).

Cheers,
Arun