Address code review

Rename SIZE_MAX and SIZE_MIN to SSIZE_MAX and SSIZE_MIN, and the
PySlice_GetIndicesEx() wrappers to slice_getindicesex_macro() and
slice_getindicesex_func().  Test the limited C API also on free threading.
Reduce the number of tested combinations.  Add tests for zero and negative
length, and document that the length must not be negative.

Author: serhiy-storchaka

Doc/c-api/slice.rst

@@ -53,6 +53,7 @@ Slice Objects
    length *length*, and store the length of the slice in *slicelength*.  Out
    of bounds indices are clipped in a manner consistent with the handling of
    normal slices.
+   *length* must not be negative.
 
    Return ``0`` on success and ``-1`` on error with an exception set.
 
@@ -108,6 +109,7 @@ Slice Objects
    Out of bounds indices are clipped in a manner consistent with the handling
    of normal slices.
 
+   *length* must not be negative.
    *step* must not be zero and must not be less than ``-PY_SSIZE_T_MAX``,
    as guaranteed by :c:func:`PySlice_Unpack`.
 

Lib/test/test_capi/test_slice.py

@@ -5,12 +5,12 @@
 _testlimitedcapi = import_helper.import_module('_testlimitedcapi')
 
 NULL = None
-SIZE_MAX = sys.maxsize
-SIZE_MIN = -sys.maxsize - 1
+SSIZE_MAX = sys.maxsize
+SSIZE_MIN = -sys.maxsize - 1
 
-VALUES = [None, 0, 1, 2, 3, 5, 7, -1, -2, -3, -5, -7]
-STEPS = [None, 1, 2, 3, 5, -1, -2, -3, -5]
-LENGTHS = [0, 1, 2, 5, 10]
+VALUES = [None, 0, 1, 3, 7, -1, -3, -7]
+STEPS = [None, 1, 3, 5, -1, -3, -5]
+LENGTHS = [0, 1, 3, 10]
 
 
 class Index:
@@ -76,11 +76,11 @@ def test_getindices(self):
         self.assertIsNone(getindices(slice(1, 'a'), 10))
         self.assertIsNone(getindices(slice(1, 7, 'a'), 10))
 
-        # negative length
-        self.assertIsNone(getindices(slice(None), -1))
-        self.assertIsNone(getindices(slice(1, 7, 2), -1))
-        self.assertEqual(getindices(slice(-10, -5, 1), -1), (-11, -6, 1))
-        self.assertEqual(getindices(slice(-5, -10, -2), -1), (-6, -11, -2))
+        # Negative length is not supported, but does not fail.
+        self.assertIsNone(getindices(slice(None), -3))
+        self.assertIsNone(getindices(slice(1, 7, 2), -3))
+        self.assertEqual(getindices(slice(-10, -5, 1), -3), (-13, -8, 1))
+        self.assertEqual(getindices(slice(-5, -10, -2), -3), (-8, -13, -2))
 
         # CRASHES getindices(NULL, 10)
         # CRASHES getindices(object(), 10)
@@ -92,27 +92,27 @@ def test_unpack(self):
         self.assertEqual(unpack(slice(1, 7, 2)), (1, 7, 2))
         self.assertEqual(unpack(slice(7, 1, -2)), (7, 1, -2))
         self.assertEqual(unpack(slice(None, 7, 2)), (0, 7, 2))
-        self.assertEqual(unpack(slice(None, 7, -2)), (SIZE_MAX, 7, -2))
-        self.assertEqual(unpack(slice(1, None, 2)), (1, SIZE_MAX, 2))
-        self.assertEqual(unpack(slice(1, None, -2)), (1, SIZE_MIN, -2))
-        self.assertEqual(unpack(slice(None)), (0, SIZE_MAX, 1))
+        self.assertEqual(unpack(slice(None, 7, -2)), (SSIZE_MAX, 7, -2))
+        self.assertEqual(unpack(slice(1, None, 2)), (1, SSIZE_MAX, 2))
+        self.assertEqual(unpack(slice(1, None, -2)), (1, SSIZE_MIN, -2))
+        self.assertEqual(unpack(slice(None)), (0, SSIZE_MAX, 1))
         self.assertEqual(unpack(slice(None, None, -1)),
-                         (SIZE_MAX, SIZE_MIN, -1))
+                         (SSIZE_MAX, SSIZE_MIN, -1))
         # Negative indices are not adjusted.
         self.assertEqual(unpack(slice(-3, -1)), (-3, -1, 1))
         self.assertEqual(unpack(slice(Index(1), Index(7), Index(2))),
                          (1, 7, 2))
 
         # Values which do not fit in Py_ssize_t are silently clipped.
-        self.assertEqual(unpack(slice(1, 2**1000)), (1, SIZE_MAX, 1))
-        self.assertEqual(unpack(slice(1, -2**1000)), (1, SIZE_MIN, 1))
-        self.assertEqual(unpack(slice(2**1000, 7)), (SIZE_MAX, 7, 1))
-        self.assertEqual(unpack(slice(-2**1000, 7)), (SIZE_MIN, 7, 1))
-        self.assertEqual(unpack(slice(1, 7, 2**1000)), (1, 7, SIZE_MAX))
+        self.assertEqual(unpack(slice(1, 2**1000)), (1, SSIZE_MAX, 1))
+        self.assertEqual(unpack(slice(1, -2**1000)), (1, SSIZE_MIN, 1))
+        self.assertEqual(unpack(slice(2**1000, 7)), (SSIZE_MAX, 7, 1))
+        self.assertEqual(unpack(slice(-2**1000, 7)), (SSIZE_MIN, 7, 1))
+        self.assertEqual(unpack(slice(1, 7, 2**1000)), (1, 7, SSIZE_MAX))
         # The step is boosted to -PY_SSIZE_T_MAX, not PY_SSIZE_T_MIN, so
         # that negating it is safe.
-        self.assertEqual(unpack(slice(7, 1, -2**1000)), (7, 1, -SIZE_MAX))
-        self.assertEqual(unpack(slice(7, 1, SIZE_MIN)), (7, 1, -SIZE_MAX))
+        self.assertEqual(unpack(slice(7, 1, -2**1000)), (7, 1, -SSIZE_MAX))
+        self.assertEqual(unpack(slice(7, 1, SSIZE_MIN)), (7, 1, -SSIZE_MAX))
 
         with self.assertRaisesRegex(ValueError, 'slice step cannot be zero'):
             unpack(slice(1, 1, 0))
@@ -153,8 +153,8 @@ def test_adjustindices(self):
         # Out of bounds indices are clipped.
         self.assertEqual(adjust(10, -100, 100, 1), (10, 0, 10))
         self.assertEqual(adjust(10, 100, -100, -1), (10, 9, -1))
-        self.assertEqual(adjust(10, SIZE_MIN, SIZE_MAX, 1), (10, 0, 10))
-        self.assertEqual(adjust(10, SIZE_MAX, SIZE_MIN, -1), (10, 9, -1))
+        self.assertEqual(adjust(10, SSIZE_MIN, SSIZE_MAX, 1), (10, 0, 10))
+        self.assertEqual(adjust(10, SSIZE_MAX, SSIZE_MIN, -1), (10, 9, -1))
         self.assertEqual(adjust(0, 1, 7, 1), (0, 0, 0))
         self.assertEqual(adjust(0, 7, 1, -1), (0, -1, -1))
 
@@ -170,18 +170,24 @@ def test_adjustindices(self):
                             self.assertEqual(slicelength,
                                              len(range(start2, stop2, step)))
 
+        # Negative length is not supported, but does not fail.
+        self.assertEqual(adjust(-3, 1, 7, 1), (0, -3, -3))
+        self.assertEqual(adjust(-3, 7, 1, -1), (0, -4, -4))
+        self.assertEqual(adjust(-3, -10, -5, 1), (0, 0, 0))
+
         # The step is asserted to be neither zero nor less than
         # -PY_SSIZE_T_MAX.
         # CRASHES adjust(10, 0, 10, 0)
-        # CRASHES adjust(10, 0, 10, SIZE_MIN)
+        # CRASHES adjust(10, 0, 10, SSIZE_MIN)
 
 
-class GetIndicesExTest(unittest.TestCase):
+class GetIndicesExMacroTest(unittest.TestCase):
     # PySlice_GetIndicesEx() is a macro using PySlice_Unpack() and
     # PySlice_AdjustIndices().  It is also a deprecated function, exported
     # for the stable ABI.
-    getindicesex = staticmethod(_testlimitedcapi.slice_getindicesex)
-    getindicesex_seq = staticmethod(_testlimitedcapi.slice_getindicesex_seq)
+    getindicesex = staticmethod(_testlimitedcapi.slice_getindicesex_macro)
+    getindicesex_seq = staticmethod(
+        _testlimitedcapi.slice_getindicesex_seq_macro)
     # The macro evaluates the length after calling PySlice_Unpack(), so the
     # size of the list after removing an item is used.
     resized = (6, 8, 1, 2)
@@ -215,18 +221,21 @@ def test_getindicesex(self):
         self.assertEqual(getindicesex(slice(100, -100, -1), 10),
                          (9, -1, -1, 10))
         self.assertEqual(getindicesex(slice(None), 0), (0, 0, 1, 0))
+        self.assertEqual(getindicesex(slice(1, 7, 2), 0), (0, 0, 2, 0))
+        self.assertEqual(getindicesex(slice(None, None, -1), 0),
+                         (-1, -1, -1, 0))
 
         # Indices which do not fit in Py_ssize_t are clipped, not rejected.
         # Note that slice.indices() does not clip the step.
         self.assertEqual(getindicesex(slice(1, 2**1000), 10), (1, 10, 1, 9))
         self.assertEqual(getindicesex(slice(2**1000, 7), 10), (10, 7, 1, 0))
         self.assertEqual(getindicesex(slice(1, 7, 2**1000), 10),
-                         (1, 7, SIZE_MAX, 1))
+                         (1, 7, SSIZE_MAX, 1))
         # -PY_SSIZE_T_MAX-1 is replaced with -PY_SSIZE_T_MAX.
         self.assertEqual(getindicesex(slice(7, 1, -2**1000), 10),
-                         (7, 1, -SIZE_MAX, 1))
-        self.assertEqual(getindicesex(slice(7, 1, SIZE_MIN), 10),
-                         (7, 1, -SIZE_MAX, 1))
+                         (7, 1, -SSIZE_MAX, 1))
+        self.assertEqual(getindicesex(slice(7, 1, SSIZE_MIN), 10),
+                         (7, 1, -SSIZE_MAX, 1))
 
         with self.assertRaisesRegex(ValueError, 'slice step cannot be zero'):
             getindicesex(slice(1, 7, 0), 10)
@@ -246,6 +255,11 @@ def test_getindicesex(self):
         with self.assertRaisesRegex(RuntimeError, 'bad index'):
             getindicesex(slice(1, 7, BadIndex()), 10)
 
+        # Negative length is not supported, but does not fail.
+        self.assertEqual(getindicesex(slice(None), -3), (-3, -3, 1, 0))
+        self.assertEqual(getindicesex(slice(1, 7, 2), -3), (-3, -3, 2, 0))
+        self.assertEqual(getindicesex(slice(7, 1, -2), -3), (-4, -4, -2, 0))
+
         # CRASHES getindicesex(NULL, 10)
         # CRASHES getindicesex(object(), 10)
 
@@ -254,6 +268,7 @@ def test_getindicesex_seq(self):
         getindicesex_seq = self.getindicesex_seq
         seq = list(range(10))
         self.assertEqual(getindicesex_seq(slice(-3, -1), seq), (7, 9, 1, 2))
+        self.assertEqual(getindicesex_seq(slice(-3, -1), []), (0, 0, 1, 0))
 
         # gh-72054: __index__() can resize the sequence.  Negative indices
         # are adjusted by the length, so the result depends on when it is
@@ -278,12 +293,12 @@ def test_getindicesex_seq(self):
         # CRASHES getindicesex_seq(slice(None), object())
 
 
-class GetIndicesExDeprecatedTest(GetIndicesExTest):
+class GetIndicesExFuncTest(GetIndicesExMacroTest):
     # The deprecated function is equivalent to the macro, except that the
     # length is evaluated before the call.
-    getindicesex = staticmethod(_testlimitedcapi.slice_getindicesex_deprecated)
+    getindicesex = staticmethod(_testlimitedcapi.slice_getindicesex_func)
     getindicesex_seq = staticmethod(
-        _testlimitedcapi.slice_getindicesex_seq_deprecated)
+        _testlimitedcapi.slice_getindicesex_seq_func)
     # The size of the list before removing an item is used.
     resized = (7, 9, 1, 2)
 

Modules/_testlimitedcapi/slice.c

@@ -1,8 +1,9 @@
 #include "pyconfig.h"   // Py_GIL_DISABLED
-
-// Need limited C API 3.6.1 for PySlice_Unpack() and PySlice_AdjustIndices()
-// and for PySlice_GetIndicesEx() implemented as a macro.
-#if !defined(Py_GIL_DISABLED) && !defined(Py_LIMITED_API)
+#ifdef Py_GIL_DISABLED
+#  define Py_TARGET_ABI3T 0x030f0000
+#else
+   // Need limited C API 3.6.1 for PySlice_Unpack() and PySlice_AdjustIndices()
+   // and for PySlice_GetIndicesEx() implemented as a macro.
 #  define Py_LIMITED_API 0x03060100
 #endif
 
@@ -62,7 +63,7 @@ slice_getindices(PyObject *Py_UNUSED(module), PyObject *args)
 /* Test PySlice_GetIndicesEx() implemented as a macro using PySlice_Unpack()
  * and PySlice_AdjustIndices(). */
 static PyObject *
-slice_getindicesex(PyObject *Py_UNUSED(module), PyObject *args)
+slice_getindicesex_macro(PyObject *Py_UNUSED(module), PyObject *args)
 {
     PyObject *slice;
     Py_ssize_t length = UNINITIALIZED_SIZE;
@@ -90,11 +91,11 @@ slice_getindicesex(PyObject *Py_UNUSED(module), PyObject *args)
     return Py_BuildValue("nnnn", start, stop, step, slicelength);
 }
 
-/* Same as slice_getindicesex(), but the length is the size of a sequence.
+/* Same as slice_getindicesex_macro(), but the length is the size of a sequence.
  * The macro evaluates it after calling PySlice_Unpack(), which can execute
  * arbitrary Python code and resize the sequence. */
 static PyObject *
-slice_getindicesex_seq(PyObject *Py_UNUSED(module), PyObject *args)
+slice_getindicesex_seq_macro(PyObject *Py_UNUSED(module), PyObject *args)
 {
     PyObject *slice, *seq;
     Py_ssize_t start = UNINITIALIZED_SIZE;
@@ -154,7 +155,7 @@ slice_adjustindices(PyObject *Py_UNUSED(module), PyObject *args)
 /* Test the deprecated PySlice_GetIndicesEx() function.  It is still exported
  * for the stable ABI and used if Py_LIMITED_API is older than 3.5.4. */
 static PyObject *
-slice_getindicesex_deprecated(PyObject *Py_UNUSED(module), PyObject *args)
+slice_getindicesex_func(PyObject *Py_UNUSED(module), PyObject *args)
 {
     PyObject *slice;
     Py_ssize_t length;
@@ -186,10 +187,10 @@ _Py_COMP_DIAG_POP
 }
 
 
-/* Same as slice_getindicesex_seq(), but using the deprecated function.
+/* Same as slice_getindicesex_seq_macro(), but using the deprecated function.
  * The length is evaluated before the call. */
 static PyObject *
-slice_getindicesex_seq_deprecated(PyObject *Py_UNUSED(module), PyObject *args)
+slice_getindicesex_seq_func(PyObject *Py_UNUSED(module), PyObject *args)
 {
     PyObject *slice, *seq;
     Py_ssize_t start = UNINITIALIZED_SIZE;
@@ -221,11 +222,11 @@ static PyMethodDef test_methods[] = {
     {"slice_check", slice_check, METH_O},
     {"slice_new", slice_new, METH_VARARGS},
     {"slice_getindices", slice_getindices, METH_VARARGS},
-    {"slice_getindicesex", slice_getindicesex, METH_VARARGS},
-    {"slice_getindicesex_seq", slice_getindicesex_seq, METH_VARARGS},
-    {"slice_getindicesex_deprecated", slice_getindicesex_deprecated,
+    {"slice_getindicesex_macro", slice_getindicesex_macro, METH_VARARGS},
+    {"slice_getindicesex_seq_macro", slice_getindicesex_seq_macro, METH_VARARGS},
+    {"slice_getindicesex_func", slice_getindicesex_func,
      METH_VARARGS},
-    {"slice_getindicesex_seq_deprecated", slice_getindicesex_seq_deprecated,
+    {"slice_getindicesex_seq_func", slice_getindicesex_seq_func,
      METH_VARARGS},
     {"slice_unpack", slice_unpack, METH_O},
     {"slice_adjustindices", slice_adjustindices, METH_VARARGS},