[PATCH 1 of 3 v3] mpatch: shuffle stuff around so it's easier to make a cffi version of mpatch
Maciej Fijalkowski
fijall at gmail.com
Thu Jul 7 03:02:43 EDT 2016
So how do you guys want this to be split?
On Wed, Jul 6, 2016 at 8:03 PM, Sean Farley <sean at farley.io> wrote:
> Yuya Nishihara <yuya at tcha.org> writes:
>
>> 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.
>
> Thanks to Yuya for trying to review this.
>
> To be honest, I'm fairly put off by the patch author insisting on mixing
> code and design changes. This is at least the third time I've seen an
> error when trying to do both. Which is a problem because it exhausts too
> much of a reviewer's time.
>
> So, I'm asking: please, please, with a cherry on top, split up your
> patches for review.
More information about the Mercurial-devel
mailing list