alsa-lib's new API issue (snd_ctl_elem_id_compare)

[Takashi Sakamoto]

Hi,

On Fri, Mar 12, 2021 at 11:09:42AM +0100, Jaroslav Kysela wrote:
> Dne 12. 03. 21 v 2:35 Takashi Sakamoto napsal(a):
> > On Thu, Mar 11, 2021 at 02:22:45PM +0100, Jaroslav Kysela wrote:
> >>> Hm. I believe that you agree with the fact that we can make various
> >>> algorithms to compare a pair of IDs for control elements. When focusing
> >>> on fields except for numid, we can make the other algorithms against your
> >>> implementation, since the ID structure is compound one. Each of the
> >>> algorithms can return different result.
> >>>
> >>> Here, I'd like to shift the discussion to the name of new API. Although it
> >>> has the most common name, 'snd_ctl_id_compare', it just has one of
> >>> comparison algorithms. I have a concern that the name can gives wrong idea
> >>> to users that the ID structure for control element had design to be able to
> >>> be compared by itself and it would just be a single comparison algorithm.
> >>>
> >>> I suggest to rename the new API to express that it implements one of
> >>> comparison algorithm. In a case of numid comparison, it would be
> >>> 'snd_ctl_id_compare_by_numid()'. For your case,
> >>> 'snd_ctl_id_compare_by_name_arithmetic' or something suitable.
> >>
> >> Perhaps, we can add a third argument defining the sorting algorithm, so we
> >> don't bloat the symbol tables so much when we add a new sorting type (enum).
> >> It would mean that the function cannot be used as a direct argument to
> >> qsort(), but I think that the apps add usually an extra code to own callback
> >> depending on containers, anyway. Is it more appropriate for you?
> > 
> > I've already investigated the idea you describe, however I concluded
> > that it has more complexity than convenience.
> > 
> > For example, the prototype would be:
> > 
> > ```
> > int new_api(const snd_ctl_elem_id_t *l, const snd_ctl_elem_id_t *r,
> >             int (*algorithm)(const snd_ctl_elem_id_t *,
> >                              const snd_ctl_elem_id_t *));
> > ```
> > 
> > For usage with qsort_r(3), programmer should do:
> > 
> > ```
> > int my_algo(const snd_ctl_elem_id_t *l, snd_ctl_elem_id_t *r)
> > {
> >    ...
> > }
> > 
> > qsort_r(base, nmemb, size, new_api, my_algo);
> > ```
> 
> I meant:
> 
> ```
> int new_api(const snd_ctl_elem_id_t *id1, const const snd_ctl_elem_id_t *id2,
>             snd_ctl_compare_type_t ctype)
> {
>    ...
> }
> 
> int my_algo(void *a, void *b)
> {
> 	struct mystruct *my1 = a;
> 	struct mystruct *my2 = b;
> 	... possible extra code ...
> 	return new_api(&my1->id, &my2->id, SND_CTL_COMPARE_FULL_WO_NUMID);
> }
> 
> qsort(base, nmemb, size, my_algo);
> 
> int my_algo_r(void *a, void *b, void *opaque)
> {
> 	struct config *cfg = opaque;
> 	struct mystruct *my1 = a;
> 	struct mystruct *my2 = b;
> 	.. possibe extra code ..
> 	return new_api(&my1->id, &my2->id, cfg->sort_type);
> }
> 
> qsort_r(base, nmemb, size, my_algo_r, cfg);
> ```
> 
> So I don't see a real drawback in the real use. Of course, each API has some
> pros and cons,

Yep. In alsa-utils, you just use the new API to check equality of a pair
of IDs for control element. At present, there are no codes to call the
new API for sorting purpose.

> but I think that mine is easier for the common cases than the set of functions.

Although I agree that your implementation for the comparison algorithm is
useful, it's just one of some useful algorithms. It is difficult to
handle your algorithm as the most valuable, specific, unique one apart from
the other ones. The name, 'snd_ctl_elem_id_compare' is inappropriate for the
case, since it could exclude possibility to implement the other useful
algorithms.

Actually you need to describe the exclusiveness into the function comment,
"The numid comparison can be easily implemented using snd_ctl_elem_id_get_numid()
calls."[1]. And it interfaces to produce comparison algorithm by numid field as
optimized function (=done by single function call, without two function call
to get numid from opaque pointers).

> The two argument functions can be used directly only with qsort() anyway.

At present, it's a better compromise to implement the set of functions
exposed to applications for each algorithm, I think. When we got many
functions, then start to discuss about the unified-style function for
sorting.

As a first step, let me rename your implementation, then add another
implementation to compare numid field. What do you think about it?


[1] https://github.com/alsa-project/alsa-lib/commit/266618088aa6e17672ffb08a110b2fff2e2f7ad2

Regards

Takashi Sakamoto