Re: [PATCH 1/2] Move git_sort(), a stable sort, into into libgit.a

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

 



"Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
writes:

> ---
>  Makefile                  | 2 +-
>  compat/mingw.c            | 5 -----
>  git-compat-util.h         | 4 +++-
>  compat/qsort.c => qsort.c | 2 +-
>  4 files changed, 5 insertions(+), 8 deletions(-)
>  rename compat/qsort.c => qsort.c (97%)

Quite pleasing.

> diff --git a/compat/mingw.c b/compat/mingw.c
> index 738f0a826a..77d4ef4d19 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1229,11 +1229,6 @@ static int wenvcmp(const void *a, const void *b)
>  	return _wcsnicmp(p, q, p_len);
>  }
>  
> -/* We need a stable sort to convert the environment between UTF-16 <-> UTF-8 */
> -#ifndef INTERNAL_QSORT
> -#include "qsort.c"
> -#endif
> -

Especially these ;-)

> diff --git a/compat/qsort.c b/qsort.c
> similarity index 97%
> rename from compat/qsort.c
> rename to qsort.c
> index 7d071afb70..08f80eea09 100644
> --- a/compat/qsort.c
> +++ b/qsort.c
> @@ -1,4 +1,4 @@
> -#include "../git-compat-util.h"
> +#include "git-compat-util.h"

I however do not think this goes far enough.  With a bit more effort
would make the intention of the API more obvious.

What we are saying now is that

 (1) some platforms do not even have qsort()
 (2) some codepaths do care the stability of sorting

the former used to be the reason why we called our implementation
git_qsort() and aliased qsort() to use git_qsort().

But now (2) is in the picture, we would probably want to make it
more clear that our own implementation is not about having a sort
function that behaves like qsort() and We want a lot more out of it
(namely, stability).  It probably is time to rename git_qsort() to
git_stable_qsort() or something like that.

Macros QSORT() and QSORT_S() are about having a convenient and less
error-prone thin wrapper that retains an interface similar to
qsort(3) and qsort_s(3) that developers are familiar with, so they
can and should stay the same name, and it is perfectly fine if the
former called qsort() that in turn calls git_stable_qsort() on some
platforms.

And that way, if/when the calling site of QSORT() (not the calling
site of STABLE_QSORT()) needs a more performant but unstable sort
implementation, we can safely give the most sensible name for it,
which is git_qsort().

Other than that, the two patches look good to me.  Thanks for
working on this topic.






[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]

  Powered by Linux