[PATCH 1 of 2] parsers: simplify error logic in compute_phases_map_sets
Yuya Nishihara
yuya at tcha.org
Sun Dec 13 01:30:52 CST 2015
On Sat, 12 Dec 2015 20:11:50 -0800, Bryan O'Sullivan wrote:
> # HG changeset patch
> # User Bryan O'Sullivan <bos at serpentine.com>
> # Date 1449979901 28800
> # Sat Dec 12 20:11:41 2015 -0800
> # Node ID 6c0dd7cd82797705c5be51be892afd94a4909a65
> # Parent 18f300e9ca45cdae3575d1860251929abf07b42c
> parsers: simplify error logic in compute_phases_map_sets
>
> Since Py_XDECREF and free both accept NULL pointers, we can get by
> with just two exit paths: one for success, and one for error.
>
> This considerably simplifies reasoning about the possible ways to
> exit from this function.
and fixes memleak if phasessetlist == NULL.
> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> --- a/mercurial/parsers.c
> +++ b/mercurial/parsers.c
> @@ -1281,19 +1281,19 @@ static PyObject *compute_phases_map_sets
> long phase;
>
> if (!PyArg_ParseTuple(args, "O", &roots))
> - goto release_none;
> + goto done;
> if (roots == NULL || !PyList_Check(roots))
> - goto release_none;
> + goto done;
> phases = calloc(len, 1); /* phase per rev: {0: public, 1: draft, 2: secret} */
> if (phases == NULL)
> - goto release_none;
> + goto done;
Oops, I found PyErr_NoMemory() was missing here.
> /* Put the phase information of all the roots in phases */
> numphase = PyList_GET_SIZE(roots)+1;
> minrevallphases = len + 1;
> phasessetlist = PyList_New(numphase);
> if (phasessetlist == NULL)
> - goto release_none;
> + goto done;
>From your comment, "one for success, one for error", I think these "goto done"
should be "goto release" even though there are no difference.
> ret = PyList_New(2);
> if (ret == NULL)
> - goto release_phaseslist;
> + goto release;
>
> PyList_SET_ITEM(ret, 0, phaseslist);
> PyList_SET_ITEM(ret, 1, phasessetlist);
> /* We don't release phaseslist and phasessetlist as we return them to
> * python */
> - goto release_phases;
> + goto done;
This is off topic, but perhaps it can be written by PyTuple_Pack().
> -release_phaseslist:
> +release:
> Py_XDECREF(phaseslist);
> -release_phasesetlist:
> Py_XDECREF(phasessetlist);
> -release_phases:
> +done:
> free(phases);
> -release_none:
> return ret;
> }
More information about the Mercurial-devel
mailing list