Title: specialcase simple sliceobj in list (and bugfixes)
Type: Stage:
Components: Interpreter Core Versions: Python 2.6
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: twouters Nosy List: paulhankin, twouters
Priority: normal Keywords: patch

Created on 2006-12-18 03:35 by twouters, last changed 2007-08-28 15:36 by twouters. This issue is now closed.

File name Uploaded Description Edit
noslice.list.diff twouters, 2006-12-18 03:35
Messages (3)
msg51563 - (view) Author: Thomas Wouters (twouters) * (Python committer) Date: 2006-12-18 03:35
 - Specialcase the step=1/None case of slicing using sliceobjects.
 - Fix bug where slice insertion using a sliceobject was inserting in a different place than slice inserting using a simple slice
 - Fix two off-by-one bugs that were cancelling each other out: the non-step-1 slice deletion code was memmoving too much for each item to delete, and not enough for the tail end, net result being a few too many bytes moved for each item.
 - Rewrite tail end move when deleting complex slice, use single memmove() instead of repeated PyList_SET_ITEM()
 - Expand comments describing the code somewhat.
msg51564 - (view) Author: Paul Hankin (paulhankin) Date: 2007-03-11 16:25
An alternative to the tail memmove would just be to incorporate it in the main loop; something like (untested):

for(i=0; i<slicelength; i++){
    size_t target = start + i * (step - 1);
    size_t source = start + i * step + 1;
    size_t source_end = i < slicelength - 1 ? source + step - 1
                                            : self->ob_size;
    garbage[i] = PyList_GET_ITEM(self, start + i * step);
    assert(source_end <= self->ob_size);
    memmove(&self->ob_item[target], &self->ob_item[source],
        (source_end - source) * sizeof(PyObject *));

I think this also makes it more obvious what's going on - at the moment the code smells a bit funny, even with your new comment.

Otherwise, the change looks good.
msg55373 - (view) Author: Thomas Wouters (twouters) * (Python committer) Date: 2007-08-28 15:36
I prefer the current method, as it's more obviously walking in two
strides across the same array. I also dislike hiding the final memmove()
of the tail bit inside the loop. As for which is more obvious, I would
submit neither is obvious, as it took me quite a bit of brainsweat to
figure out how either version was supposed to work after not looking at
the code for months :)

Committed revision 57619.
Date User Action Args
2007-08-28 15:36:04twouterssetstatus: open -> closed
assignee: twouters
resolution: fixed
messages: + msg55373
2006-12-18 03:35:42twouterscreate