revlog: address review feedback for deltachain C implementation
authorGregory Szorc <gregory.szorc@gmail.com>
Sat, 01 Jul 2017 19:35:17 -0700
changeset 33174 f4f52bb362e6
parent 33173 4b0da963586d
child 33175 3b85c474cbcf
revlog: address review feedback for deltachain C implementation * Scope of "value" is reduced * index_baserev() is documented * Error is no longer redundantly set for -2 return values * Error values are compared <= -2 instead of == -2 to protect against odd failure scenarios
mercurial/cext/revlog.c
--- a/mercurial/cext/revlog.c	Sat Jul 01 15:13:09 2017 -0400
+++ b/mercurial/cext/revlog.c	Sat Jul 01 19:35:17 2017 -0700
@@ -8,6 +8,7 @@
 */
 
 #include <Python.h>
+#include <assert.h>
 #include <ctype.h>
 #include <stddef.h>
 #include <string.h>
@@ -816,6 +817,11 @@
 	return NULL;
 }
 
+/**
+ * Obtain the base revision index entry.
+ *
+ * Callers must ensure that rev >= 0 or illegal memory access may occur.
+ */
 static inline int index_baserev(indexObject *self, int rev)
 {
 	const char *data;
@@ -841,7 +847,7 @@
 	PyObject *stoparg;
 	int stoprev, iterrev, baserev = -1;
 	int stopped;
-	PyObject *chain = NULL, *value = NULL, *result = NULL;
+	PyObject *chain = NULL, *result = NULL;
 	const Py_ssize_t length = index_length(self);
 
 	if (!PyArg_ParseTuple(args, "iOi", &rev, &stoparg, &generaldelta)) {
@@ -876,15 +882,16 @@
 	baserev = index_baserev(self, rev);
 
 	/* This should never happen. */
-	if (baserev == -2) {
-		PyErr_SetString(PyExc_IndexError, "unable to resolve data");
+	if (baserev <= -2) {
+		/* Error should be set by index_deref() */
+		assert(PyErr_Occurred());
 		goto bail;
 	}
 
 	iterrev = rev;
 
 	while (iterrev != baserev && iterrev != stoprev) {
-		value = PyInt_FromLong(iterrev);
+		PyObject *value = PyInt_FromLong(iterrev);
 		if (value == NULL) {
 			goto bail;
 		}
@@ -913,8 +920,9 @@
 		baserev = index_baserev(self, iterrev);
 
 		/* This should never happen. */
-		if (baserev == -2) {
-			PyErr_SetString(PyExc_IndexError, "unable to resolve data");
+		if (baserev <= -2) {
+			/* Error should be set by index_deref() */
+			assert(PyErr_Occurred());
 			goto bail;
 		}
 	}
@@ -923,7 +931,7 @@
 		stopped = 1;
 	}
 	else {
-		value = PyInt_FromLong(iterrev);
+		PyObject *value = PyInt_FromLong(iterrev);
 		if (value == NULL) {
 			goto bail;
 		}