grflib off-by-one error in grf_sort()

[ultramage]

Hi, while working on the grf code in trunk/grftool/lib/grfsupport.c I noticed one small bug that I'm fairly certain about. Posting it here because... well, why not.

Before, the grf_sort() function looked like this:

Code: Select all

static void GRF_qsort (Grf *grf, uint32_t left, uint32_t right, GrfSortCallback callback);

GRFEXPORT void grf_sort (Grf *grf, GrfSortCallback callback)
{
    GRF_qsort(grf, 0, grf->nfiles-1, callback);
}

In r1309 the custom internal quicksort algorithm was thrown out in favor of just calling libc's qsort().

Code: Select all

void qsort( void * base, size_t num, size_t size, int ( * comparator ) ( const void *, const void * ) );

GRFEXPORT void grf_sort (Grf *grf, int(*compar)(const void *, const void *))
{
    qsort(grf->files, grf->nfiles-1, sizeof(GrfFile), compar);
}

I am pointing out that the person who made this change did not transition the call arguments from "offsets" (left=0 and right=nfiles-1) to "number of entries". He left in nfiles-1, which is indeed the last entry, but since qsort() wants the count, I believe this should have been "nfiles" instead. If I am right, then the consequence is that the last entry in the array does not get sorted.

I only noticed this by looking at the code; I do not know if this incomplete sort has any effect on the correctness of the library. The comments in grf_find_unused() explicitly warn about the array being sorted, however I think it will just cause a potentially useable empty block to not be used when saving new data.

[kLabMouse]

I personally thing that there should be grf->nfiles
But in reality, it can mask another bug, that could reveal while trying to fix it.