[PATCH 1 of 3 v3] mpatch: shuffle stuff around so it's easier to make a cffi version of mpatch

[Yuya Nishihara]

On Mon, 04 Jul 2016 11:16:23 +0200, Maciej Fijalkowski wrote:
> # HG changeset patch
> # User Maciej Fijalkowski <fijall at gmail.com>
> # Date 1465303444 -7200
> #      Tue Jun 07 14:44:04 2016 +0200
> # Node ID 5c7e2b441d644a544c8fd26dfa3d0d50cb966307
> # Parent  3c349a3d45b909dbad0506365344ee7e1471026a
> mpatch: shuffle stuff around so it's easier to make a cffi version of mpatch

> --- a/mercurial/compat.h	Mon Jun 06 13:08:13 2016 +0200
> +++ b/mercurial/compat.h	Tue Jun 07 14:44:04 2016 +0200
> @@ -35,4 +35,8 @@
>  #define inline __inline
>  #endif
>  
> +#ifndef ssize_t
> +#define ssize_t size_t
>  #endif

Wrong.

 - ssize_t isn't a macro. #ifndef can't be used
 - ssize_t should be signed, but size_t is unsigned

Can you take a look how CPython handle the definition of Py_ssize_t?

> --- a/mercurial/mpatch.c	Mon Jun 06 13:08:13 2016 +0200
> +++ b/mercurial/mpatch.c	Tue Jun 07 14:44:04 2016 +0200

> +static struct mpatch_flist *lalloc(ssize_t size)
>  {
> -	struct flist *a = NULL;
> +	struct mpatch_flist *a = NULL;
>  
>  	if (size < 1)
>  		size = 1;
>  
> -	a = (struct flist *)malloc(sizeof(struct flist));
> +	a = (struct mpatch_flist *)malloc(sizeof(struct mpatch_flist));
>  	if (a) {
> -		a->base = (struct frag *)malloc(sizeof(struct frag) * size);
> +		a->base = (struct mpatch_frag *)malloc(sizeof(struct mpatch_frag) *
> +			size);
>  		if (a->base) {
>  			a->head = a->tail = a->base;
>  			return a;
> @@ -57,12 +47,10 @@
>  		free(a);
>  		a = NULL;
>  	}
> -	if (!PyErr_Occurred())
> -		PyErr_NoMemory();

Now it's the responsibility of the caller to set PyErr_NoMemory, but

> +struct mpatch_flist *cpygetnextitem(void *bins, ssize_t pos)
> +{
> +	const char *buffer;
> +	struct mpatch_flist *res;
> +	ssize_t blen;
> +	int r;
> +
> +	PyObject *tmp = PyList_GetItem((PyObject*)bins, pos);
> +	if (!tmp)
> +		return NULL;
> +	if (PyObject_AsCharBuffer(tmp, &buffer, (Py_ssize_t*)&blen))
> +		return NULL;
> +	if ((r = mpatch_decode(buffer, blen, &res)) < 0) {
> +		if (!PyErr_Occurred())
> +			PyErr_SetString(mpatch_Error, mpatch_errors[-r]);
> +		return NULL;
> +	}
> +	return res;
> +}

new fold() sets mpatch_Error instead,

> +static PyObject *
> +patches(PyObject *self, PyObject *args)
> +{
> +	PyObject *text, *bins, *result;
> +	struct mpatch_flist *patch;
> +	const char *in;
> +	char *out;
> +	Py_ssize_t len, outlen, inlen;
> +
> +	if (!PyArg_ParseTuple(args, "OO:mpatch", &text, &bins))
> +		return NULL;
> +
> +	len = PyList_Size(bins);
> +	if (!len) {
> +		/* nothing to do */
> +		Py_INCREF(text);
> +		return text;
> +	}
> +
> +	if (PyObject_AsCharBuffer(text, &in, &inlen))
> +		return NULL;
> +
> +	patch = mpatch_fold(bins, cpygetnextitem, 0, len);
> +	if (!patch) {
> +		return NULL;

or return NULL without setting any exception.

I haven't read the code other than lalloc(). I'll need more time to concentrate
on reviewing this. To reviewers, feel free to beat me if you have time.

And again, I'm not a fan of mixing code moves, naming changes, design changes,
etc. into one patch just because they are things necessary for the cffi mpatch.