From: René Scharfe <l.s.r@web.de>
Date: Thu, 29 Dec 2016 13:53:54 GMT
Subject: Re: [PATCH] string-list: make compare function compatible with qsort(3)
Message-ID: <43c3d83e-80f1-64ea-2ba5-7540b8dd4910@web.de>
In-Reply-To: <20161221161220.x3qkcwmuangcdc2l@sigill.intra.peff.net>

Am 21.12.2016 um 17:12 schrieb Jeff King:
> On Wed, Dec 21, 2016 at 10:36:41AM +0100, René Scharfe wrote:
>
>> One shortcoming is that the comparison function is restricted to working
>> with the string members of items; util is inaccessible to it.  Another
>> one is that the value of cmp is passed in a global variable to
>> cmp_items(), making string_list_sort() non-reentrant.
>
> I think this approach is OK for string_list, but it doesn't help the
> general case that wants qsort_s() to actually access global data. I
> don't know how common that is in our codebase, though.
>
> So I'm fine with it, but I think we might eventually need to revisit the
> qsort_s() thing anyway.

I have to admit I didn't even consider the possibility that the pattern 
of accessing global variables in qsort(1) compare functions could have 
spread.

And indeed, at least ref-filter.c::compare_refs() and 
worktree.c::compare_worktree() so that as well.  The latter uses 
fspathcmp(), which is OK as ignore_case is only set once when reading 
the config, but the first one looks, well, interesting.  Perhaps a 
single ref filter per program is enough?

Anyway, that's a good enough argument for me for adding that newfangled 
C11 function after all..

Btw.: Found with

   git grep 'QSORT.*;$' |
   sed 's/.* /int /; s/);//' |
   sort -u |
   git grep -Ww -f-

and

   git grep -A1 'QSORT.*,$'

>> Remove the intermediate layer, i.e. cmp_items(), make the comparison
>> functions compatible with qsort(3) and pass them pointers to full items.
>> This allows comparisons to also take the util member into account, and
>> avoids the need to pass the real comparison function to an intermediate
>> function, removing the need for a global function.
>
> I'm not sure if access to the util field is really of any value, after
> looking at it in:
>
>   http://public-inbox.org/git/20161125171546.fa3zpapbjngjcl26@sigill.intra.peff.net/
>
> Though note that if we do take this patch, there are probably one or two
> spots that could switch from QSORT() to string_list_sort().

Yes, but as you noted in that thread there is not much point in doing 
that; the only net win is that we can pass a list as a single pointer 
instead of as base pointer and element count -- the special compare 
function needs to be specified anyway (once), somehow.

René