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

[Peter Meerwald]

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

regards, p.

-- 

Peter Meerwald
+43-664-2444418 (mobile)