Re: alsa-lib's new API issue (snd_ctl_elem_id_compare)

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/266618088aa6e17672ffb08a110b...

Regards

Takashi Sakamoto