Re: [PATCH] string-list: make compare function compatible with qsort(3)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxxxxx/

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é



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]