- 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.
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.
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.
History
Date
User
Action
Args
2022-04-11 14:56:21
admin
set
github: 44351
2007-08-28 15:36:04
twouters
set
status: open -> closedassignee: twoutersresolution: fixedmessages: +
