Re: t5505-remote fails on Windows

From: Jeff King <peff@peff.net>
Date: Mar 19, 2009, 20:03 UTC
Message-ID: <20090319200308.GB17028@coredump.intra.peff.net>
In-Reply-To: <7vhc1pai84.fsf@gitster.siamese.dyndns.org>

On Thu, Mar 19, 2009 at 04:02:19AM -0700, Junio C Hamano wrote:

Show 11 quoted lines

> > Do we really want an API for that?  Calling qsort() directly should be 
> > obvious enough, no?
> 
> I think so.  If it were done like this (notice the lack of double
> indirection in the cmp_fn signature):
> 
>     typedef int string_list_item_cmp_fn(const struct string_list_item *, const struct string_list_item *);
> 
>     void sort_string_list_with_fn(struct string_list *list, string_list_item_cmp_fn *);
> 
> it would have made more sense, though.

IIRC, that is actually not valid C according to the standard (that is, even though a void* can be implicitly assigned to any other pointer, a function taking a void* and a function taking another pointer do not necessarily have the same function signature or calling conventions). Which is why cmp_items in string-list.c already does the indirection.

That being said, I think this is probably one of those cases where it works fine on any sane platform that we care about and we can choose to ignore the standard.

Actually, I think an even nicer API would be to give a function that _just_ compares the util fields; string_list_sort() would sort on the "string" fields, and only call the util cmp to sort entries with equal strings. Sadly, because qsort lacks a slot for extra data to pass to the callback, implementing this ends up with a global variable:

  static int (*cmp_util)(const void *, const void *);

  int cmp_items(const void *a, const void *b)
  {
    const struct string_list_item *one = a;
    const struct string_list_item *two = b;
    int cmp = strcmp(one->string, two->string);
    return cmp ? cmp : cmp_util(one->util, b->util);
  }

  int sort_string_list_with_util(struct string_list *list,
                       int (*fn)(const void *, const void *))
  {
    cmp_util = fn;
    qsort(list->items, list->nr, sizeof(*list->items), cmp_items);
  }

If only C had closures. ;)

But this is really getting beyond the issue at hand. I can submit a finalized patch; just let me know how you prefer it.

-Peff