list_sort se comporta extraño al ordenar mediante más de un comparador

[RaniAgus]

Hace unos días vi este comentario en la función list_sort, entonces me puse a investigar y a probar algoritmos mejores. https://github.com/sisoputnfrba/so-commons-library/blob/1c885402ddbf4cd2feb7d9d879ba439c5ca12c23/src/commons/collections/list.c#L234

En medio de todo esto, vi que no había ningún test que permita chequear si list_sort acepta más de un criterio (dicho en criollo, un criterio "de desempate"), así que hice uno:

static bool _ayudantes_alfabetico(t_person *primero, t_person *segundo) {
    return strcmp(primero->name, segundo->name) < 0;
}

                void _verify_a_sort_with_multiple_comparators(t_list* (*sorted_list_generator)()) {
                    list_add(list, persona_create("Ezequiel", 25));
                    list_add(list, persona_create("Julian" , 25));
                    list_add(list, persona_create("Facundo" , 25));
                    list_sort(list, (void*) _ayudantes_alfabetico);

                    t_list* a_sorted_list = sorted_list_generator();
                    assert_person_in_list(a_sorted_list, 0, "Daniela"  , 19);
                    assert_person_in_list(a_sorted_list, 1, "Sebastian", 21);
                    assert_person_in_list(a_sorted_list, 2, "Matias"   , 24);
                    assert_person_in_list(a_sorted_list, 3, "Ezequiel" , 25); //<- se agregó
                    assert_person_in_list(a_sorted_list, 4, "Facundo"  , 25); //<- se agregó
                    assert_person_in_list(a_sorted_list, 5, "Gaston"   , 25);
                    assert_person_in_list(a_sorted_list, 6, "Julian"   , 25); //<- se agregó
                }

Y los resultados me dieron que la función ordena exactamente al revés de lo que debería:

    1) Sort - multiple comparators - should sort a list with multiple comparators
      - Expected <"Ezequiel"> but was <"Julian"> [test_list.c:54]
      - Expected <"Facundo"> but was <"Gaston"> [test_list.c:54]
      - Expected <"Gaston"> but was <"Facundo"> [test_list.c:54]
      - Expected <"Julian"> but was <"Ezequiel"> [test_list.c:54]

Me parece que esto pasa porque el comparador devuelve true solo si "el primero debe ir antes que el segundo", y en casos de "empate" esto da false, por lo que se efectúa el intercambio. Como resultado, termina quedando al revés de como debería

Yo propongo usar comparadores que funcionen como strcmp, que devuelven un valor negativo si el primero va antes, cero si son iguales, y positivo si el segundo va antes.

Solo haría falta cambiar el tipo del comparador a int (en las 2 funciones que lo utilizan) y esta línea: https://github.com/sisoputnfrba/so-commons-library/blob/1c885402ddbf4cd2feb7d9d879ba439c5ca12c23/src/commons/collections/list.c#L246 por esta:

if(comparator(previous_element->data, cursor->data) > 0) {

Es más, para mí esto permitiría que el comparador sea más expresivo en cuanto a lo que hace, porque para comparar strings se usaría strcmp directo:

static int _ayudantes_alfabetico(t_person *primero, t_person *segundo) {
    return strcmp(primero->name, segundo->name);
}

y para valores numéricos se haría una diferencia:

static int _ayudantes_menor(t_person *joven, t_person *menos_joven) {
    return joven->age - menos_joven->age;
}

en lugar de usar booleanos.

¿Qué opinan?

[RaniAgus]

Cierro el issue porque me acabo de dar cuenta que no es un bug en sí, sino que es un mal uso de las commons, debí usar un comparador que incluya el = para marcar que el primer elemento va siempre antes que el segundo:

static bool _ayudantes_alfabetico(t_person *primero, t_person *segundo) {
    return strcmp(primero->name, segundo->name) <= 0;
}