Sequence methods, fixing sq_ass_item with some very weird index manipulation for negative indexes.

paulross · paulross · commit 018f3515c38e · 2025-03-18T11:36:33.000Z
diff --git a/doc/sphinx/source/new_types.rst b/doc/sphinx/source/new_types.rst
@@ -942,24 +942,79 @@ In ``src/cpy/Object/cSeqObject.c``:
 
 .. code-block:: c
 
-    /** Returns a new reference to an indexed item in a sequence. */
-    static PyObject *
-    SequenceLongObject_sq_item(PyObject *self, Py_ssize_t index) {
+    static int
+    SequenceLongObject_sq_ass_item(PyObject *self, Py_ssize_t index, PyObject *value) {
+        fprintf(
+            stdout, "%s()#%d: self=%p index=%zd value=%p\n",
+            __FUNCTION__, __LINE__, (void *) self, index, (void *) value
+        );
         Py_ssize_t my_index = index;
         if (my_index < 0) {
             my_index += SequenceLongObject_sq_length(self);
         }
         // Corner case example: len(self) == 0 and index < 0
+        fprintf(
+            stdout, "%s()#%d: len=%zd index=%zd my_index=%zd\n", __FUNCTION__, __LINE__,
+            SequenceLongObject_sq_length(self), index, my_index
+        );
         if (my_index < 0 || my_index >= SequenceLongObject_sq_length(self)) {
             PyErr_Format(
                     PyExc_IndexError,
                     "Index %ld is out of range for length %ld",
                     index,
                     SequenceLongObject_sq_length(self)
             );
-            return NULL;
+            return -1;
         }
-        return PyLong_FromLong(((SequenceLongObject *) self)->array_long[my_index]);
+        if (value != NULL) {
+            /* Just set the value. */
+            if (!PyLong_Check(value)) {
+                PyErr_Format(
+                        PyExc_TypeError,
+                        "sq_ass_item value needs to be an int, not type %s",
+                        Py_TYPE(value)->tp_name
+                );
+                return -1;
+            }
+            ((SequenceLongObject *) self)->array_long[my_index] = PyLong_AsLong(value);
+        } else {
+            /* Delete the value. */
+            /* For convenience. */
+            SequenceLongObject *self_as_slo = (SequenceLongObject *) self;
+            /* Special case: deleting the only item in the array. */
+            if (self_as_slo->size == 1) {
+                fprintf(stdout, "%s()#%d: deleting empty index\n", __FUNCTION__, __LINE__);
+                free(self_as_slo->array_long);
+                self_as_slo->array_long = NULL;
+                self_as_slo->size = 0;
+            } else {
+                /* Delete the value and re-compose the array. */
+                fprintf(stdout, "%s()#%d: deleting index=%zd\n", __FUNCTION__, __LINE__, index);
+                long *new_array = malloc((self_as_slo->size - 1) * sizeof(long));
+                if (!new_array) {
+                    PyErr_Format(
+                            PyExc_MemoryError,
+                            "sq_ass_item can not allocate new array. %s#%d",
+                            __FILE__, __LINE__
+                    );
+                    return -1;
+                }
+                /* Copy up to the index. */
+                Py_ssize_t index_new_array = 0;
+                for (Py_ssize_t i = 0; i < my_index; ++i, ++index_new_array) {
+                    new_array[index_new_array] = self_as_slo->array_long[i];
+                }
+                /* Copy past the index. */
+                for (Py_ssize_t i = my_index + 1; i < self_as_slo->size; ++i, ++index_new_array) {
+                    new_array[index_new_array] = self_as_slo->array_long[i];
+                }
+
+                free(self_as_slo->array_long);
+                self_as_slo->array_long = new_array;
+                --self_as_slo->size;
+            }
+        }
+        return 0;
     }
 
 Tests
@@ -971,52 +1026,7 @@ Tests are in ``tests/unit/test_c_seqobject.py`` which includes failure modes:
 
     from cPyExtPatt import cSeqObject
 
-    @pytest.mark.parametrize(
-        'initial_sequence, index, expected',
-        (
-                (
-                        [7, 4, 1, ], 0, 7,
-                ),
-                (
-                        [7, 4, 1, ], 1, 4,
-                ),
-                (
-                        [7, 4, 1, ], 2, 1,
-                ),
-                (
-                        [7, 4, 1, ], -1, 1,
-                ),
-                (
-                        [7, 4, 1, ], -2, 4,
-                ),
-                (
-                        [7, 4, 1, ], -3, 7,
-                ),
-        )
-    )
-    def test_SequenceLongObject_item(initial_sequence, index, expected):
-        obj = cSeqObject.SequenceLongObject(initial_sequence)
-        assert obj[index] == expected
-
-    @pytest.mark.parametrize(
-        'initial_sequence, index, expected',
-        (
-                (
-                        [], 0, 'Index 0 is out of range for length 0',
-                ),
-                (
-                        [], -1, 'Index -1 is out of range for length 0',
-                ),
-                (
-                        [1, ], 2, 'Index 2 is out of range for length 1',
-                ),
-        )
-    )
-    def test_SequenceLongObject_item_raises(initial_sequence, index, expected):
-        obj = cSeqObject.SequenceLongObject(initial_sequence)
-        with pytest.raises(IndexError) as err:
-            obj[index]
-        assert err.value.args[0] == expected
+    pass
 
 
 
diff --git a/src/cpy/Object/cSeqObject.c b/src/cpy/Object/cSeqObject.c
@@ -222,11 +222,24 @@ SequenceLongObject_sq_ass_item(PyObject *self, Py_ssize_t index, PyObject *value
         stdout, "%s()#%d: self=%p index=%zd value=%p\n",
         __FUNCTION__, __LINE__, (void *) self, index, (void *) value
     );
+    /* This is very weird. */
+    if (index < 0) {
+        fprintf(
+            stdout, "%s()#%d: Fixing index index=%zd to %zd\n", __FUNCTION__, __LINE__,
+            index, index - SequenceLongObject_sq_length(self)
+        );
+        index -= SequenceLongObject_sq_length(self);
+    }
+    /* Isn't it? */
     Py_ssize_t my_index = index;
     if (my_index < 0) {
         my_index += SequenceLongObject_sq_length(self);
     }
     // Corner case example: len(self) == 0 and index < 0
+    fprintf(
+        stdout, "%s()#%d: len=%zd index=%zd my_index=%zd\n", __FUNCTION__, __LINE__,
+        SequenceLongObject_sq_length(self), index, my_index
+    );
     if (my_index < 0 || my_index >= SequenceLongObject_sq_length(self)) {
         PyErr_Format(
                 PyExc_IndexError,
@@ -269,38 +282,16 @@ SequenceLongObject_sq_ass_item(PyObject *self, Py_ssize_t index, PyObject *value
                 );
                 return -1;
             }
-            /* memcpy parameters. */
-            void *dest = NULL;
-            void *src = NULL;
-            size_t count = 0;
-
-            /* memcpy across to the new array, firstly up to the index. */
-            dest = new_array;
-            src = self_as_slo->array_long;
-            count = my_index * sizeof(long);
-            fprintf(stdout, "%s()#%d: First: dest=%p src=%p count=%zu\n", __FUNCTION__, __LINE__, dest, src, count);
-            if (memcpy(dest, src, count) != dest) {
-                PyErr_Format(
-                        PyExc_MemoryError,
-                        "sq_ass_item can not memcpy into new array. %s#%d",
-                        __FILE__, __LINE__
-                );
-                return -1;
+            /* Copy up to the index. */
+            Py_ssize_t index_new_array = 0;
+            for (Py_ssize_t i = 0; i < my_index; ++i, ++index_new_array) {
+                new_array[index_new_array] = self_as_slo->array_long[i];
             }
-            /* memcpy from the index to the end. */
-            dest = new_array + count;
-            src = self_as_slo->array_long + count;
-            /* Example size=4, index=2 copy one value */
-            count = (self_as_slo->size - my_index - 1) * sizeof(long);
-            fprintf(stdout, "%s()#%d:  Next: dest=%p src=%p count=%zu\n", __FUNCTION__, __LINE__, dest, src, count);
-            if (memcpy(dest, src, count) != dest) {
-                PyErr_Format(
-                        PyExc_MemoryError,
-                        "sq_ass_item can not memcpy into new array. %s#%d",
-                        __FILE__, __LINE__
-                );
-                return -1;
+            /* Copy past the index. */
+            for (Py_ssize_t i = my_index + 1; i < self_as_slo->size; ++i, ++index_new_array) {
+                new_array[index_new_array] = self_as_slo->array_long[i];
             }
+
             free(self_as_slo->array_long);
             self_as_slo->array_long = new_array;
             --self_as_slo->size;
@@ -309,15 +300,15 @@ SequenceLongObject_sq_ass_item(PyObject *self, Py_ssize_t index, PyObject *value
     return 0;
 }
 
-PySequenceMethods SequenceLongObject_sequence_methods = {
-        .sq_length = &SequenceLongObject_sq_length,
-        .sq_concat = &SequenceLongObject_sq_concat,
-        .sq_repeat = &SequenceLongObject_sq_repeat,
-        .sq_item = &SequenceLongObject_sq_item,
-        .sq_ass_item = &SequenceLongObject_sq_ass_item,
-        .sq_contains = NULL,
-        .sq_inplace_concat = NULL,
-        .sq_inplace_repeat = NULL,
+static PySequenceMethods SequenceLongObject_sequence_methods = {
+        .sq_length = (lenfunc)SequenceLongObject_sq_length,
+        .sq_concat = (binaryfunc)SequenceLongObject_sq_concat,
+        .sq_repeat = (ssizeargfunc)SequenceLongObject_sq_repeat,
+        .sq_item = (ssizeargfunc)SequenceLongObject_sq_item,
+        .sq_ass_item = (ssizeobjargproc)SequenceLongObject_sq_ass_item,
+        .sq_contains = (objobjproc)NULL,
+        .sq_inplace_concat = (binaryfunc)NULL,
+        .sq_inplace_repeat = (ssizeargfunc)NULL,
 };
 
 static PyObject *
diff --git a/tests/unit/test_c_seqobject.py b/tests/unit/test_c_seqobject.py
@@ -156,47 +156,100 @@ def test_SequenceLongObject_item_raises(initial_sequence, index, expected):
             (
                     [7, 4, 1, ], -1, 14, [7, 4, 14, ],
             ),
-            # (
-            #         [], 0, None, [],
-            # ),
+    )
+)
+def test_SequenceLongObject_setitem(initial_sequence, index, value, expected):
+    obj = cSeqObject.SequenceLongObject(initial_sequence)
+    obj[index] = value
+    assert list(obj) == expected
+
+
+@pytest.mark.parametrize(
+    'initial_sequence, index, expected',
+    (
             (
-                    [7,], 0, None, [],
+                    [7, 4, 1, ], 3, 'Index 3 is out of range for length 3',
             ),
             (
-                    [7,], -1, None, [],
+                    [7, 4, 1, ], -4, 'Index -4 is out of range for length 3',
             ),
+    )
+)
+def test_SequenceLongObject_setitem_raises(initial_sequence, index, expected):
+    print()
+    print(initial_sequence, index, expected)
+    obj = cSeqObject.SequenceLongObject(initial_sequence)
+    with pytest.raises(IndexError) as err:
+        obj[index] = 100
+        print(list(obj))
+    assert err.value.args[0] == expected
+
+
+@pytest.mark.parametrize(
+    'initial_sequence, index, expected',
+    (
             (
-                    [7, 4, 1, ], 1, None, [7, 1, ],
+                    [7,], 0, [],
             ),
             (
-                    [7, 4, ], 0, None, [4, ],
+                    [7,], -1, [],
+            ),
+            (
+                    [7, 4, 1, ], 1, [7, 1, ],
+            ),
+            (
+                    [7, 4, ], 0, [4, ],
             ),
     )
 )
-def test_SequenceLongObject_setitem(initial_sequence, index, value, expected):
+def test_SequenceLongObject_delitem(initial_sequence, index, expected):
     obj = cSeqObject.SequenceLongObject(initial_sequence)
-    if value is not None:
-        obj[index] = value
-    else:
-        del obj[index]
+    del obj[index]
     assert list(obj) == expected
 
 
 @pytest.mark.parametrize(
-    'initial_sequence, index, value, expected',
+    'initial_sequence, index, expected',
     (
             (
-                    [7, 4, 1, ], 1, None, [7, 1, ],
+                    [], 0, 'Index 0 is out of range for length 0',
+            ),
+            (
+                    [], -1, 'Index -1 is out of range for length 0',
+            ),
+            (
+                    [7,], 1, 'Index 1 is out of range for length 1',
+            ),
+            (
+                    [7,], -3, 'Index -3 is out of range for length 1',
             ),
     )
 )
-def test_SequenceLongObject_setitem_debug(initial_sequence, index, value, expected):
+def test_SequenceLongObject_delitem_raises(initial_sequence, index, expected):
+    print()
+    print(initial_sequence, index, expected)
     obj = cSeqObject.SequenceLongObject(initial_sequence)
-    if value is not None:
-        obj[index] = value
-    else:
+    print(list(obj))
+    with pytest.raises(IndexError) as err:
         del obj[index]
-    assert list(obj) == expected
+    assert err.value.args[0] == expected
+
+
+# @pytest.mark.parametrize(
+#     'initial_sequence, index, value, expected',
+#     (
+#             (
+#                     [7, 4, 1, ], 1, None, [7, 1, ],
+#             ),
+#     )
+# )
+# def test_SequenceLongObject_setitem_debug(initial_sequence, index, value, expected):
+#     obj = cSeqObject.SequenceLongObject(initial_sequence)
+#     if value is not None:
+#         obj[index] = value
+#     else:
+#         del obj[index]
+#     assert list(obj) == expected
 
 
 # @pytest.mark.skipif(not (sys.version_info.minor < 7), reason='Python < 3.7')
