From ffdb2c21b34253077001a0181c2fe1f4e4b2be15 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Sat, 17 Nov 2012 19:18:10 +0000 Subject: [PATCH] Issue #16451: Refactor to remove duplication between range and slice in slice index computations. --- Include/sliceobject.h | 3 + Objects/rangeobject.c | 205 ++---------------------------------------- Objects/sliceobject.c | 148 +++++++++++++++--------------- 3 files changed, 87 insertions(+), 269 deletions(-) diff --git a/Include/sliceobject.h b/Include/sliceobject.h index 8bec17909d6..f7ee90c3cc8 100644 --- a/Include/sliceobject.h +++ b/Include/sliceobject.h @@ -34,6 +34,9 @@ PyAPI_FUNC(PyObject *) PySlice_New(PyObject* start, PyObject* stop, PyObject* step); #ifndef Py_LIMITED_API PyAPI_FUNC(PyObject *) _PySlice_FromIndices(Py_ssize_t start, Py_ssize_t stop); +PyAPI_FUNC(int) _PySlice_GetLongIndices(PySliceObject *self, PyObject *length, + PyObject **start_ptr, PyObject **stop_ptr, + PyObject **step_ptr); #endif PyAPI_FUNC(int) PySlice_GetIndices(PyObject *r, Py_ssize_t length, Py_ssize_t *start, Py_ssize_t *stop, Py_ssize_t *step); diff --git a/Objects/rangeobject.c b/Objects/rangeobject.c index 214b4556b54..ba51fec9e43 100644 --- a/Objects/rangeobject.c +++ b/Objects/rangeobject.c @@ -318,195 +318,6 @@ range_item(rangeobject *r, Py_ssize_t i) return res; } -/* Additional helpers, since the standard slice helpers - * all clip to PY_SSIZE_T_MAX - */ - -/* Replace _PyEval_SliceIndex */ -static PyObject * -compute_slice_element(PyObject *obj) -{ - PyObject *result = NULL; - if (obj != NULL) { - if (PyIndex_Check(obj)) { - result = PyNumber_Index(obj); - } - else { - PyErr_SetString(PyExc_TypeError, - "slice indices must be integers or " - "None or have an __index__ method"); - } - } - return result; -} - -/* Replace PySlice_GetIndicesEx - * Result indicates whether or not the slice is empty - * (-1 = error, 0 = empty slice, 1 = slice contains elements) - */ -static int -compute_slice_indices(rangeobject *r, PySliceObject *slice, - PyObject **start, PyObject **stop, PyObject **step) -{ - int cmp_result, has_elements; - Py_ssize_t clamped_step = 0; - PyObject *zero = NULL, *one = NULL, *neg_one = NULL, *candidate = NULL; - PyObject *tmp_start = NULL, *tmp_stop = NULL, *tmp_step = NULL; - zero = PyLong_FromLong(0); - if (zero == NULL) goto Fail; - one = PyLong_FromLong(1); - if (one == NULL) goto Fail; - neg_one = PyLong_FromLong(-1); - if (neg_one == NULL) goto Fail; - - /* Calculate step value */ - if (slice->step == Py_None) { - clamped_step = 1; - tmp_step = one; - Py_INCREF(tmp_step); - } else { - if (!_PyEval_SliceIndex(slice->step, &clamped_step)) goto Fail; - if (clamped_step == 0) { - PyErr_SetString(PyExc_ValueError, - "slice step cannot be zero"); - goto Fail; - } - tmp_step = compute_slice_element(slice->step); - if (tmp_step == NULL) goto Fail; - } - - /* Calculate start value */ - if (slice->start == Py_None) { - if (clamped_step < 0) { - tmp_start = PyNumber_Subtract(r->length, one); - if (tmp_start == NULL) goto Fail; - } else { - tmp_start = zero; - Py_INCREF(tmp_start); - } - } else { - candidate = compute_slice_element(slice->start); - if (candidate == NULL) goto Fail; - cmp_result = PyObject_RichCompareBool(candidate, zero, Py_LT); - if (cmp_result == -1) goto Fail; - if (cmp_result) { - /* candidate < 0 */ - tmp_start = PyNumber_Add(r->length, candidate); - if (tmp_start == NULL) goto Fail; - Py_CLEAR(candidate); - } else { - /* candidate >= 0 */ - tmp_start = candidate; - candidate = NULL; - } - cmp_result = PyObject_RichCompareBool(tmp_start, zero, Py_LT); - if (cmp_result == -1) goto Fail; - if (cmp_result) { - /* tmp_start < 0 */ - Py_CLEAR(tmp_start); - if (clamped_step < 0) { - tmp_start = neg_one; - } else { - tmp_start = zero; - } - Py_INCREF(tmp_start); - } else { - /* tmp_start >= 0 */ - cmp_result = PyObject_RichCompareBool(tmp_start, r->length, Py_GE); - if (cmp_result == -1) goto Fail; - if (cmp_result) { - /* tmp_start >= r->length */ - Py_CLEAR(tmp_start); - if (clamped_step < 0) { - tmp_start = PyNumber_Subtract(r->length, one); - if (tmp_start == NULL) goto Fail; - } else { - tmp_start = r->length; - Py_INCREF(tmp_start); - } - } - } - } - - /* Calculate stop value */ - if (slice->stop == Py_None) { - if (clamped_step < 0) { - tmp_stop = neg_one; - } else { - tmp_stop = r->length; - } - Py_INCREF(tmp_stop); - } else { - candidate = compute_slice_element(slice->stop); - if (candidate == NULL) goto Fail; - cmp_result = PyObject_RichCompareBool(candidate, zero, Py_LT); - if (cmp_result == -1) goto Fail; - if (cmp_result) { - /* candidate < 0 */ - tmp_stop = PyNumber_Add(r->length, candidate); - if (tmp_stop == NULL) goto Fail; - Py_CLEAR(candidate); - } else { - /* candidate >= 0 */ - tmp_stop = candidate; - candidate = NULL; - } - cmp_result = PyObject_RichCompareBool(tmp_stop, zero, Py_LT); - if (cmp_result == -1) goto Fail; - if (cmp_result) { - /* tmp_stop < 0 */ - Py_CLEAR(tmp_stop); - if (clamped_step < 0) { - tmp_stop = neg_one; - } else { - tmp_stop = zero; - } - Py_INCREF(tmp_stop); - } else { - /* tmp_stop >= 0 */ - cmp_result = PyObject_RichCompareBool(tmp_stop, r->length, Py_GE); - if (cmp_result == -1) goto Fail; - if (cmp_result) { - /* tmp_stop >= r->length */ - Py_CLEAR(tmp_stop); - if (clamped_step < 0) { - tmp_stop = PyNumber_Subtract(r->length, one); - if (tmp_stop == NULL) goto Fail; - } else { - tmp_stop = r->length; - Py_INCREF(tmp_stop); - } - } - } - } - - /* Check if the slice is empty or not */ - if (clamped_step < 0) { - has_elements = PyObject_RichCompareBool(tmp_start, tmp_stop, Py_GT); - } else { - has_elements = PyObject_RichCompareBool(tmp_start, tmp_stop, Py_LT); - } - if (has_elements == -1) goto Fail; - - *start = tmp_start; - *stop = tmp_stop; - *step = tmp_step; - Py_DECREF(neg_one); - Py_DECREF(one); - Py_DECREF(zero); - return has_elements; - - Fail: - Py_XDECREF(tmp_start); - Py_XDECREF(tmp_stop); - Py_XDECREF(tmp_step); - Py_XDECREF(candidate); - Py_XDECREF(neg_one); - Py_XDECREF(one); - Py_XDECREF(zero); - return -1; -} - static PyObject * compute_slice(rangeobject *r, PyObject *_slice) { @@ -514,10 +325,11 @@ compute_slice(rangeobject *r, PyObject *_slice) rangeobject *result; PyObject *start = NULL, *stop = NULL, *step = NULL; PyObject *substart = NULL, *substop = NULL, *substep = NULL; - int has_elements; + int error; - has_elements = compute_slice_indices(r, slice, &start, &stop, &step); - if (has_elements == -1) return NULL; + error = _PySlice_GetLongIndices(slice, r->length, &start, &stop, &step); + if (error == -1) + return NULL; substep = PyNumber_Multiply(r->step, step); if (substep == NULL) goto fail; @@ -527,13 +339,8 @@ compute_slice(rangeobject *r, PyObject *_slice) if (substart == NULL) goto fail; Py_CLEAR(start); - if (has_elements) { - substop = compute_item(r, stop); - if (substop == NULL) goto fail; - } else { - substop = substart; - Py_INCREF(substop); - } + substop = compute_item(r, stop); + if (substop == NULL) goto fail; Py_CLEAR(stop); result = make_range_object(Py_TYPE(r), substart, substop, substep); diff --git a/Objects/sliceobject.c b/Objects/sliceobject.c index 4b31f2306ee..52f1c89ded9 100644 --- a/Objects/sliceobject.c +++ b/Objects/sliceobject.c @@ -316,57 +316,41 @@ evaluate_slice_index(PyObject *v) } } -/* Implementation of slice.indices. */ +/* Compute slice indices given a slice and length. Return -1 on failure. Used + by slice.indices and rangeobject slicing. Assumes that `len` is a + nonnegative instance of PyLong. */ -static PyObject* -slice_indices(PySliceObject* self, PyObject* len) +int +_PySlice_GetLongIndices(PySliceObject *self, PyObject *length, + PyObject **start_ptr, PyObject **stop_ptr, + PyObject **step_ptr) { PyObject *start=NULL, *stop=NULL, *step=NULL; - PyObject *length=NULL, *upper=NULL, *lower=NULL, *zero=NULL; - int step_is_negative, cmp; + PyObject *upper=NULL, *lower=NULL; + int step_is_negative, cmp_result; - zero = PyLong_FromLong(0L); - if (zero == NULL) - return NULL; - - /* Compute step and length as integers. */ - length = PyNumber_Index(len); - if (length == NULL) - goto error; - - if (self->step == Py_None) + /* Convert step to an integer; raise for zero step. */ + if (self->step == Py_None) { step = PyLong_FromLong(1L); - else + if (step == NULL) + goto error; + step_is_negative = 0; + } + else { + int step_sign; step = evaluate_slice_index(self->step); - if (step == NULL) - goto error; - - /* Raise ValueError for negative length or zero step. */ - cmp = PyObject_RichCompareBool(length, zero, Py_LT); - if (cmp < 0) { - goto error; - } - if (cmp) { - PyErr_SetString(PyExc_ValueError, - "length should not be negative"); - goto error; - } - - cmp = PyObject_RichCompareBool(step, zero, Py_EQ); - if (cmp < 0) { - goto error; - } - if (cmp) { - PyErr_SetString(PyExc_ValueError, - "slice step cannot be zero"); - goto error; + if (step == NULL) + goto error; + step_sign = _PyLong_Sign(step); + if (step_sign == 0) { + PyErr_SetString(PyExc_ValueError, + "slice step cannot be zero"); + goto error; + } + step_is_negative = step_sign < 0; } /* Find lower and upper bounds for start and stop. */ - step_is_negative = PyObject_RichCompareBool(step, zero, Py_LT); - if (step_is_negative < 0) { - goto error; - } if (step_is_negative) { lower = PyLong_FromLong(-1L); if (lower == NULL) @@ -377,8 +361,10 @@ slice_indices(PySliceObject* self, PyObject* len) goto error; } else { - lower = zero; - Py_INCREF(lower); + lower = PyLong_FromLong(0L); + if (lower == NULL) + goto error; + upper = length; Py_INCREF(upper); } @@ -393,10 +379,7 @@ slice_indices(PySliceObject* self, PyObject* len) if (start == NULL) goto error; - cmp = PyObject_RichCompareBool(start, zero, Py_LT); - if (cmp < 0) - goto error; - if (cmp) { + if (_PyLong_Sign(start) < 0) { /* start += length */ PyObject *tmp = PyNumber_Add(start, length); Py_DECREF(start); @@ -404,20 +387,20 @@ slice_indices(PySliceObject* self, PyObject* len) if (start == NULL) goto error; - cmp = PyObject_RichCompareBool(start, lower, Py_LT); - if (cmp < 0) + cmp_result = PyObject_RichCompareBool(start, lower, Py_LT); + if (cmp_result < 0) goto error; - if (cmp) { + if (cmp_result) { Py_INCREF(lower); Py_DECREF(start); start = lower; } } else { - cmp = PyObject_RichCompareBool(start, upper, Py_GT); - if (cmp < 0) + cmp_result = PyObject_RichCompareBool(start, upper, Py_GT); + if (cmp_result < 0) goto error; - if (cmp) { + if (cmp_result) { Py_INCREF(upper); Py_DECREF(start); start = upper; @@ -435,10 +418,7 @@ slice_indices(PySliceObject* self, PyObject* len) if (stop == NULL) goto error; - cmp = PyObject_RichCompareBool(stop, zero, Py_LT); - if (cmp < 0) - goto error; - if (cmp) { + if (_PyLong_Sign(stop) < 0) { /* stop += length */ PyObject *tmp = PyNumber_Add(stop, length); Py_DECREF(stop); @@ -446,20 +426,20 @@ slice_indices(PySliceObject* self, PyObject* len) if (stop == NULL) goto error; - cmp = PyObject_RichCompareBool(stop, lower, Py_LT); - if (cmp < 0) + cmp_result = PyObject_RichCompareBool(stop, lower, Py_LT); + if (cmp_result < 0) goto error; - if (cmp) { + if (cmp_result) { Py_INCREF(lower); Py_DECREF(stop); stop = lower; } } else { - cmp = PyObject_RichCompareBool(stop, upper, Py_GT); - if (cmp < 0) + cmp_result = PyObject_RichCompareBool(stop, upper, Py_GT); + if (cmp_result < 0) goto error; - if (cmp) { + if (cmp_result) { Py_INCREF(upper); Py_DECREF(stop); stop = upper; @@ -467,23 +447,51 @@ slice_indices(PySliceObject* self, PyObject* len) } } + *start_ptr = start; + *stop_ptr = stop; + *step_ptr = step; Py_DECREF(upper); Py_DECREF(lower); - Py_DECREF(length); - Py_DECREF(zero); - return Py_BuildValue("(NNN)", start, stop, step); + return 0; error: + *start_ptr = *stop_ptr = *step_ptr = NULL; Py_XDECREF(start); Py_XDECREF(stop); Py_XDECREF(step); Py_XDECREF(upper); Py_XDECREF(lower); - Py_XDECREF(length); - Py_XDECREF(zero); - return NULL; + return -1; } +/* Implementation of slice.indices. */ + +static PyObject* +slice_indices(PySliceObject* self, PyObject* len) +{ + PyObject *start, *stop, *step; + PyObject *length; + int error; + + /* Convert length to an integer if necessary; raise for negative length. */ + length = PyNumber_Index(len); + if (length == NULL) + return NULL; + + if (_PyLong_Sign(length) < 0) { + PyErr_SetString(PyExc_ValueError, + "length should not be negative"); + Py_DECREF(length); + return NULL; + } + + error = _PySlice_GetLongIndices(self, length, &start, &stop, &step); + Py_DECREF(length); + if (error == -1) + return NULL; + else + return Py_BuildValue("(NNN)", start, stop, step); +} PyDoc_STRVAR(slice_indices_doc, "S.indices(len) -> (start, stop, stride)\n\