Fix obscure breakage (relative to 2.3) in listsort: the test for list

tim-one · tim-one · commit 51b4ade30615 · 2004-07-29T04:07:15.000Z
mutation during list.sort() used to rely on that listobject.c always
NULL'ed ob_item when ob_size fell to 0.  That's no longer true, so the
test for list mutation during a sort is no longer reliable.  Changed the
test to rely instead on that listobject.c now never NULLs-out ob_item
after (if ever) ob_item gets a non-NULL value.  This new assumption is
also documented now, as a required invariant in listobject.h.

The new assumption allowed some real simplification to some of the
hairier code in listsort(), so is a Good Thing on that count.
diff --git a/Include/listobject.h b/Include/listobject.h
@@ -30,6 +30,9 @@ typedef struct {
      *     0 <= ob_size <= allocated
      *     len(list) == ob_size
      *     ob_item == NULL implies ob_size == allocated == 0
+     *     If ob_item ever becomes non-NULL, it remains non-NULL for the
+     *         life of the list object.  The check for mutation in list.sort()
+     *         relies on this odd detail.
      */
     int allocated;
 } PyListObject;
diff --git a/Objects/listobject.c b/Objects/listobject.c
@@ -1906,7 +1906,6 @@ listsort(PyListObject *self, PyObject *args, PyObject *kwds)
 	int minrun;
 	int saved_ob_size, saved_allocated;
 	PyObject **saved_ob_item;
-	PyObject **empty_ob_item;
 	PyObject *compare = NULL;
 	PyObject *result = NULL;	/* guilty until proved innocent */
 	int reverse = 0;
@@ -1941,9 +1940,8 @@ listsort(PyListObject *self, PyObject *args, PyObject *kwds)
 	saved_ob_size = self->ob_size;
 	saved_ob_item = self->ob_item;
 	saved_allocated = self->allocated;
-	self->ob_size = 0;
-	self->ob_item = empty_ob_item = PyMem_NEW(PyObject *, 0);
-	self->allocated = 0;
+	self->ob_size = self->allocated = 0;
+	self->ob_item = NULL;
 
 	if (keyfunc != NULL) {
 		for (i=0 ; i < saved_ob_size ; i++) {
@@ -1957,18 +1955,6 @@ listsort(PyListObject *self, PyObject *args, PyObject *kwds)
 					saved_ob_item[i] = value;
 					Py_DECREF(kvpair);
 				}
-				if (self->ob_item != empty_ob_item
-				    || self->ob_size) {
-					/* If the list changed *as well* we
-					   have two errors.  We let the first
-					   one "win", but we shouldn't let
-					   what's in the list currently
-					   leak. */
-					(void)list_ass_slice(
-						self, 0, self->ob_size,
-						(PyObject *)NULL);
-				}
-
 				goto dsu_fail;
 			}
 			kvpair = build_sortwrapper(key, value);
@@ -2044,14 +2030,12 @@ listsort(PyListObject *self, PyObject *args, PyObject *kwds)
 		}
 	}
 
-	if (self->ob_item != empty_ob_item || self->ob_size) {
-		/* The user mucked with the list during the sort. */
-		(void)list_ass_slice(self, 0, self->ob_size, (PyObject *)NULL);
-		if (result != NULL) {
-			PyErr_SetString(PyExc_ValueError,
-					"list modified during sort");
-			result = NULL;
-		}
+	if (self->ob_item != NULL && result != NULL) {
+		/* The user mucked with the list during the sort,
+		 * and we don't already have another error to report.
+		 */
+		PyErr_SetString(PyExc_ValueError, "list modified during sort");
+		result = NULL;
 	}
 
 	if (reverse && saved_ob_size > 1)
@@ -2060,8 +2044,10 @@ listsort(PyListObject *self, PyObject *args, PyObject *kwds)
 	merge_freemem(&ms);
 
 dsu_fail:
-	if (self->ob_item == empty_ob_item)
-		PyMem_FREE(empty_ob_item);
+	if (self->ob_item != NULL) {
+		(void)list_ass_slice(self, 0, self->ob_size, (PyObject *)NULL);
+		PyMem_FREE(self->ob_item);
+	}
 	self->ob_size = saved_ob_size;
 	self->ob_item = saved_ob_item;
 	self->allocated = saved_allocated;
