Patchwork [1,of,4] parsers: simplify error logic in compute_phases_map_sets

login
register
mail settings
Submitter Bryan O'Sullivan
Date Dec. 14, 2015, 6:49 p.m.
Message ID <f87799029bb6c39860b9.1450118962@bryano-mbp.local>
Download mbox | patch
Permalink /patch/12032/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Bryan O'Sullivan - Dec. 14, 2015, 6:49 p.m.
# HG changeset patch
# User Bryan O'Sullivan <bos@serpentine.com>
# Date 1450118844 28800
#      Mon Dec 14 10:47:24 2015 -0800
# Node ID f87799029bb6c39860b9eede0ae54a8dfece8c01
# Parent  944af8e2eb4cddf96ba5b8a96854528b40979715
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.
Yuya Nishihara - Dec. 15, 2015, 3:32 p.m.
On Mon, 14 Dec 2015 10:49:22 -0800, Bryan O'Sullivan wrote:
> # HG changeset patch
> # User Bryan O'Sullivan <bos@serpentine.com>
> # Date 1450118844 28800
> #      Mon Dec 14 10:47:24 2015 -0800
> # Node ID f87799029bb6c39860b9eede0ae54a8dfece8c01
> # Parent  944af8e2eb4cddf96ba5b8a96854528b40979715
> 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.

I'll push 1, 2 and 4 to the clowncopter tomorrow, thanks!
Augie Fackler - Dec. 15, 2015, 6:17 p.m.
On Wed, Dec 16, 2015 at 12:32:24AM +0900, Yuya Nishihara wrote:
> On Mon, 14 Dec 2015 10:49:22 -0800, Bryan O'Sullivan wrote:
> > # HG changeset patch
> > # User Bryan O'Sullivan <bos@serpentine.com>
> > # Date 1450118844 28800
> > #      Mon Dec 14 10:47:24 2015 -0800
> > # Node ID f87799029bb6c39860b9eede0ae54a8dfece8c01
> > # Parent  944af8e2eb4cddf96ba5b8a96854528b40979715
> > 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.
>
> I'll push 1, 2 and 4 to the clowncopter tomorrow, thanks!

Per your review, I'm queueing these (they're not yet on the
clowncopter) and will push them shortly. Thanks!


> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/parsers.c b/mercurial/parsers.c
--- a/mercurial/parsers.c
+++ b/mercurial/parsers.c
@@ -1286,19 +1286,21 @@  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;
+	if (phases == NULL) {
+		PyErr_NoMemory();
+		goto done;
+	}
 	/* 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;
 
 	PyList_SET_ITEM(phasessetlist, 0, Py_None);
 	Py_INCREF(Py_None);
@@ -1307,13 +1309,13 @@  static PyObject *compute_phases_map_sets
 		phaseroots = PyList_GET_ITEM(roots, i);
 		phaseset = PySet_New(NULL);
 		if (phaseset == NULL)
-			goto release_phasesetlist;
+			goto release;
 		PyList_SET_ITEM(phasessetlist, i+1, phaseset);
 		if (!PyList_Check(phaseroots))
-			goto release_phasesetlist;
+			goto release;
 		minrevphase = add_roots_get_min(self, phaseroots, i+1, phases);
 		if (minrevphase == -2) /* Error from add_roots_get_min */
-			goto release_phasesetlist;
+			goto release;
 		minrevallphases = MIN(minrevallphases, minrevphase);
 	}
 	/* Propagate the phase information from the roots to the revs */
@@ -1322,14 +1324,14 @@  static PyObject *compute_phases_map_sets
 		for (i = minrevallphases; i < len; i++) {
 			if (index_get_parents(self, i, parents,
 					      (int)len - 1) < 0)
-				goto release_phasesetlist;
+				goto release;
 			set_phase_from_parents(phases, parents[0], parents[1], i);
 		}
 	}
 	/* Transform phase list to a python list */
 	phaseslist = PyList_New(len);
 	if (phaseslist == NULL)
-		goto release_phasesetlist;
+		goto release;
 	for (i = 0; i < len; i++) {
 		phase = phases[i];
 		/* We only store the sets of phase for non public phase, the public phase
@@ -1344,21 +1346,19 @@  static PyObject *compute_phases_map_sets
 	}
 	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;
 
-release_phaseslist:
+release:
 	Py_XDECREF(phaseslist);
-release_phasesetlist:
 	Py_XDECREF(phasessetlist);
-release_phases:
+done:
 	free(phases);
-release_none:
 	return ret;
 }