[alsa-lib][PATCH 0/6] add API of equality and comparison for a pair of control element IDs

[Jaroslav Kysela]

Dne 18. 03. 21 v 11:30 Takashi Sakamoto napsal(a):
> Hi,
> 
> This patchset is a fix for bug issued in the message thread[1].
> 
> In this development period, alsa-lib got new API as implementation for
> one of comparison algorithms to a pair of control element IDs. However,
> it has several issues.
> 
> At first, the name, 'snd_ctl_elem_id_compare()', is inappropriate since it
> implements one of comparison algorithms. The name itself implies the
> algorithm is single and unique for control element ID. However, the
> target structure, 'struct snd_ctl_elem_id', is hybrid and compound one.
> We can not find such single and unique comparison algorithm for it.
> 
> Secondary, it subtracts a pair of values in fields of 'unsigned int' type
> in storage size of the type. It brings integer overflow.

I don't think that this extra handling is really required. The unsigned /
signed conversions are well known and the overflow results with a negative
signed value. Why add more branches to the instruction chain?

> Tertiary, it has simple bug to compare subdevice field in the same structure.

Good catch.

> Essentially, equality is different from comparison. In a point of programming,

Yes, but in this case, there's no benefit to have things separated. Add inline
functions if you like to create application helpers which may be replaced by
the functions in the future. If we really need a super CPU optimized equality
check functions, we can add them later. The full compare functions must return
zero, if the values are equal.

I prefer minimal API changes here.

> implementation for comparison algorithm can have more overhead than
> implementation for equality. In this meaning, it's better to add different API
> for them.
> 
> This patchset adds new API below:
> 
>  * for equality
>    * snd_ctl_elem_id_equal_by_numid()
>    * snd_ctl_elem_id_equal_by_tuple()
>  * for each comparison algorithm
>    * snd_ctl_elem_id_compare_by_numid()
>    * snd_ctl_elem_id_compare_by_tuple_arithmetic()
>
> I've got bothered to decide the name of API for the case to use tuples.
> Here I use the word, 'tuple', which comes from documentation of alsa-lib[2].

The tuple naming is a bit new and I would prefer fields or so here. The ID is
represented by the number or group of fields. Also arithmetic suffix makes the
function name longer. The new function may use other name (if any will be
implemented later).

Also, I don't like l and r argument naming. We should follow strcmp() and
don't try to invent something new here.

Also my old function should be just renamed. No add + remove is necessary for
the modifications of the touched code.

Resume:

1) rename snd_ctl_elem_id_compare() to snd_ctl_elem_id_compare_fields()
2) add snd_ctl_elem_id_compare_by_numid()

.. optionally ...

3) add snd_ctl_elem_id_equal_by_numid() - as inline using compare fcn
4) add snd_ctl_elem_id_equal_by_fields() - also inline

					Jaroslav

-- 
Jaroslav Kysela <perex at perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.