Re: [PATCH 1/3] compat: add qsort_s()

Junio C Hamano [off-list ref] writes:

Eh, wait.  BSD and Microsoft have paramters reordered in the
callback comparison function.  I suspect that would not fly very
well.

Hmm.  We could do it like this, which may not be too bad.

#if APPLE_QSORT_R
struct apple_qsort_adapter {
	int (*user_cmp)(const void *, const void *, void *);
	void *user_ctx;
}

static int apple_qsort_adapter_cmp(void *ctx, const void *a, const void *b)
{
	struct apple_qsort_adapter *wrapper_ctx = ctx;
	return wrapper_ctx->user_cmp(a, b, wrapper_ctx->user_ctx);
}
#endif

int git_qsort_s(void *b, size_t n, size_t s,
      	   int (*cmp)(const void *, const void *, void *), void *ctx)
{
	if (!n)
		return 0;
	if (!b || !cmp)
		return -1;
#if GNU_QSORT_R
	qsort_r(b, n, s, cmp, ctx);
#elif APPLE_QSORT_R
	{
		struct appple_qsort_adapter a = { cmp, ctx };
		qsort_r(b, n, s, &a, appple_qsort_adapter_cmp);
	}
#endif
      return 0;
}

On Thu, Dec 01, 2016 at 12:22:37PM -0800, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

Eh, wait.  BSD and Microsoft have paramters reordered in the
callback comparison function.  I suspect that would not fly very
well.

Hmm.  We could do it like this, which may not be too bad.

Heh. Exactly, but I was too lazy to write it out in my other email. :)

The no-cost version would be more like:

#ifdef APPLE_QSORT_R
#define DECLARE_CMP(func) int func(void *data, const void *va, const void *vb)
#else
#define DECLARE_CMP(func) int func(const void *va, const void *vb, void *data)
#endif

and then:

  DECLARE_CMP(foocmp);
  ...

  DECLARE_CMP(foocmp)
  {
	const struct foo *a = va, *b = vb;
	... etc ...
  }

-Peff

Am 01.12.2016 um 21:22 schrieb Junio C Hamano:

Junio C Hamano [off-list ref] writes:

quoted

Eh, wait.  BSD and Microsoft have paramters reordered in the
callback comparison function.  I suspect that would not fly very
well.

Hmm.  We could do it like this, which may not be too bad.

It's kinda cool to have a bespoke compatibility layer for major 
platforms, but the more I think about it the less I can answer why we 
would want that.  Safety, reliability and performance can't be good 
reasons -- if our fallback function lacks in these regards then we have 
to improve it in any case.

Text size could be a valid reason, but the full function only adds a bit 
more than 2KB to the unstripped git binary.

The flip side is we'd build an ifdef maze that's harder to read and a 
lot more difficult to test.

What do we get in return for that additional complexity?

#if APPLE_QSORT_R
struct apple_qsort_adapter {
	int (*user_cmp)(const void *, const void *, void *);
	void *user_ctx;
}

static int apple_qsort_adapter_cmp(void *ctx, const void *a, const void *b)
{
	struct apple_qsort_adapter *wrapper_ctx = ctx;
	return wrapper_ctx->user_cmp(a, b, wrapper_ctx->user_ctx);
}
#endif

int git_qsort_s(void *b, size_t n, size_t s,
      	   int (*cmp)(const void *, const void *, void *), void *ctx)
{
	if (!n)
		return 0;
	if (!b || !cmp)
		return -1;
#if GNU_QSORT_R
	qsort_r(b, n, s, cmp, ctx);
#elif APPLE_QSORT_R
	{
		struct appple_qsort_adapter a = { cmp, ctx };
		qsort_r(b, n, s, &a, appple_qsort_adapter_cmp);
	}
#endif

Nit: The fallback for non-GNU, non-Apple systems is missing here, but 
the idea is illustrated clearly enough.

      return 0;
}

On Mon, Dec 12, 2016 at 08:51:14PM +0100, René Scharfe wrote:

It's kinda cool to have a bespoke compatibility layer for major platforms,
but the more I think about it the less I can answer why we would want that.
Safety, reliability and performance can't be good reasons -- if our fallback
function lacks in these regards then we have to improve it in any case.

There may be cases that we don't want to support because of portability
issues. E.g., if your libc has an assembly-optimized qsort() we wouldn't
want to replicate that.

I dunno. I am not that opposed to just saying "forget libc qsort, we
always use our internal one which is consistent, performant, and safe".
But when I suggested something similar for our regex library, I seem to
recall there were complaints.

-Peff

Am 12.12.2016 um 20:57 schrieb Jeff King:

On Mon, Dec 12, 2016 at 08:51:14PM +0100, René Scharfe wrote:

quoted

It's kinda cool to have a bespoke compatibility layer for major platforms,
but the more I think about it the less I can answer why we would want that.
Safety, reliability and performance can't be good reasons -- if our fallback
function lacks in these regards then we have to improve it in any case.

There may be cases that we don't want to support because of portability
issues. E.g., if your libc has an assembly-optimized qsort() we wouldn't
want to replicate that.

Offloading to GPUs might be a better example; I don't know of a libc 
that does any of that, though (yet).

I dunno. I am not that opposed to just saying "forget libc qsort, we
always use our internal one which is consistent, performant, and safe".
But when I suggested something similar for our regex library, I seem to
recall there were complaints.

Well, I'm not sure how comparable they are, but perhaps we can avoid 
compat code altogether in this case.  Patch coming in a new thread.

René