[PATCH 1 of 3 v3] mpatch: shuffle stuff around so it's easier to make a cffi version of mpatch
Yuya Nishihara
yuya at tcha.org
Wed Jul 6 10:19:30 EDT 2016
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.
More information about the Mercurial-devel
mailing list