optimise list.pop() a little more by splitting it off the generic anyobj.pop() implementation

Cython/Compiler/Optimize.py

@@ -2388,42 +2388,54 @@ class OptimizeBuiltinCalls(Visitor.MethodDispatcherTransform):
             PyrexTypes.CFuncTypeArg("index", PyrexTypes.c_long_type, None),
             ])
 
-    def _handle_simple_method_object_pop(self, node, function, args, is_unbound_method):
+    def _handle_simple_method_list_pop(self, node, function, args, is_unbound_method):
+        return self._handle_simple_method_object_pop(
+            node, function, args, is_unbound_method, is_list=True)
+
+    def _handle_simple_method_object_pop(self, node, function, args, is_unbound_method, is_list=False):
         """Optimistic optimisation as X.pop([n]) is almost always
         referring to a list.
         """
+        if not args:
+            return node
+        args = args[:]
+        if is_list:
+            type_name = 'List'
+            args[0] = args[0].as_none_safe_node(
+                "'NoneType' object has no attribute '%s'",
+                error="PyExc_AttributeError",
+                format_args=['pop'])
+        else:
+            type_name = 'Object'
         if len(args) == 1:
             return ExprNodes.PythonCapiCallNode(
-                node.pos, "__Pyx_PyObject_Pop", self.PyObject_Pop_func_type,
-                args = args,
-                may_return_none = True,
-                is_temp = node.is_temp,
-                utility_code = load_c_utility('pop')
-                )
+                node.pos, "__Pyx_Py%s_Pop" % type_name,
+                self.PyObject_Pop_func_type,
+                args=args,
+                may_return_none=True,
+                is_temp=node.is_temp,
+                utility_code=load_c_utility('pop'),
+            )
         elif len(args) == 2:
-            index = args[1]
-            if isinstance(index, ExprNodes.CoerceToPyTypeNode):
-                index = index.arg
-            if isinstance(index, ExprNodes.IntNode):
-                index = index.coerce_to(PyrexTypes.c_py_ssize_t_type, None)
+            index = unwrap_coerced_node(args[1])
+            if is_list or isinstance(index, ExprNodes.IntNode):
+                index = index.coerce_to(PyrexTypes.c_py_ssize_t_type, self.current_env())
             if index.type.is_int:
                 widest = PyrexTypes.widest_numeric_type(
                     index.type, PyrexTypes.c_py_ssize_t_type)
                 if widest == PyrexTypes.c_py_ssize_t_type:
                     args[1] = index
                     return ExprNodes.PythonCapiCallNode(
-                        node.pos, "__Pyx_PyObject_PopIndex",
+                        node.pos, "__Pyx_Py%s_PopIndex" % type_name,
                         self.PyObject_PopIndex_func_type,
-                        args = args,
-                        may_return_none = True,
-                        is_temp = node.is_temp,
-                        utility_code = load_c_utility("pop_index")
-                        )
+                        args=args,
+                        may_return_none=True,
+                        is_temp=node.is_temp,
+                        utility_code=load_c_utility("pop_index"),
+                    )
 
         return node
 
-    _handle_simple_method_list_pop = _handle_simple_method_object_pop
-
     single_param_func_type = PyrexTypes.CFuncType(
         PyrexTypes.c_returncode_type, [
             PyrexTypes.CFuncTypeArg("obj", PyrexTypes.py_object_type, None),

Cython/Utility/Optimize.c

@@ -76,24 +76,31 @@ static CYTHON_INLINE int __Pyx_PyList_Extend(PyObject* L, PyObject* v) {
 
 /////////////// pop.proto ///////////////
 
-static CYTHON_INLINE PyObject* __Pyx_PyObject_Pop(PyObject* L); /*proto*/
+#define __Pyx_PyObject_Pop(L) (PyList_CheckExact(L) ? \
+    __Pyx_PyList_Pop(L) : __Pyx__PyObject_Pop(L))
+
+static CYTHON_INLINE PyObject* __Pyx_PyList_Pop(PyObject* L); /*proto*/
+static CYTHON_INLINE PyObject* __Pyx__PyObject_Pop(PyObject* L); /*proto*/
 
 /////////////// pop ///////////////
 //@requires: ObjectHandling.c::PyObjectCallMethod
 
-static CYTHON_INLINE PyObject* __Pyx_PyObject_Pop(PyObject* L) {
+static CYTHON_INLINE PyObject* __Pyx__PyObject_Pop(PyObject* L) {
+#if CYTHON_COMPILING_IN_CPYTHON && PY_VERSION_HEX >= 0x02050000
+    if (Py_TYPE(L) == &PySet_Type) {
+        return PySet_Pop(L);
+    }
+#endif
+    return __Pyx_PyObject_CallMethod0(L, PYIDENT("pop"));
+}
+
+static CYTHON_INLINE PyObject* __Pyx_PyList_Pop(PyObject* L) {
 #if CYTHON_COMPILING_IN_CPYTHON && PY_VERSION_HEX >= 0x02040000
-    if (likely(PyList_CheckExact(L))
-        /* Check that both the size is positive and no reallocation shrinking needs to be done. */
-        && likely(PyList_GET_SIZE(L) > (((PyListObject*)L)->allocated >> 1))) {
+    /* Check that both the size is positive and no reallocation shrinking needs to be done. */
+    if (likely(PyList_GET_SIZE(L) > (((PyListObject*)L)->allocated >> 1))) {
         Py_SIZE(L) -= 1;
         return PyList_GET_ITEM(L, PyList_GET_SIZE(L));
     }
-#if PY_VERSION_HEX >= 0x02050000
-    else if (Py_TYPE(L) == (&PySet_Type)) {
-        return PySet_Pop(L);
-    }
-#endif
 #endif
     return __Pyx_PyObject_CallMethod0(L, PYIDENT("pop"));
 }
@@ -101,31 +108,17 @@ static CYTHON_INLINE PyObject* __Pyx_PyObject_Pop(PyObject* L) {
 
 /////////////// pop_index.proto ///////////////
 
-static PyObject* __Pyx_PyObject_PopIndex(PyObject* L, Py_ssize_t ix); /*proto*/
+#define __Pyx_PyObject_PopIndex(L, ix) (PyList_CheckExact(L) ? \
+    __Pyx_PyList_PopIndex(L, ix) : __Pyx__PyObject_PopIndex(L, ix))
+
+static PyObject* __Pyx_PyList_PopIndex(PyObject* L, Py_ssize_t ix); /*proto*/
+static PyObject* __Pyx__PyObject_PopIndex(PyObject* L, Py_ssize_t ix); /*proto*/
 
 /////////////// pop_index ///////////////
 //@requires: ObjectHandling.c::PyObjectCallMethod
 
-static PyObject* __Pyx_PyObject_PopIndex(PyObject* L, Py_ssize_t ix) {
+static PyObject* __Pyx__PyObject_PopIndex(PyObject* L, Py_ssize_t ix) {
     PyObject *r, *py_ix;
-#if CYTHON_COMPILING_IN_CPYTHON
-    if (likely(PyList_CheckExact(L))) {
-        Py_ssize_t size = PyList_GET_SIZE(L);
-        if (likely(size > (((PyListObject*)L)->allocated >> 1))) {
-            Py_ssize_t cix = ix;
-            if (cix < 0) {
-                cix += size;
-            }
-            if (likely(0 <= cix && cix < size)) {
-                PyObject* v = PyList_GET_ITEM(L, cix);
-                Py_SIZE(L) -= 1;
-                size -= 1;
-                memmove(&PyList_GET_ITEM(L, cix), &PyList_GET_ITEM(L, cix+1), (size-cix)*sizeof(PyObject*));
-                return v;
-            }
-        }
-    }
-#endif
     py_ix = PyInt_FromSsize_t(ix);
     if (!py_ix) return NULL;
     r = __Pyx_PyObject_CallMethod1(L, PYIDENT("pop"), py_ix);
@@ -133,6 +126,26 @@ static PyObject* __Pyx_PyObject_PopIndex(PyObject* L, Py_ssize_t ix) {
     return r;
 }
 
+static PyObject* __Pyx_PyList_PopIndex(PyObject* L, Py_ssize_t ix) {
+#if CYTHON_COMPILING_IN_CPYTHON
+    Py_ssize_t size = PyList_GET_SIZE(L);
+    if (likely(size > (((PyListObject*)L)->allocated >> 1))) {
+        Py_ssize_t cix = ix;
+        if (cix < 0) {
+            cix += size;
+        }
+        if (likely(0 <= cix && cix < size)) {
+            PyObject* v = PyList_GET_ITEM(L, cix);
+            Py_SIZE(L) -= 1;
+            size -= 1;
+            memmove(&PyList_GET_ITEM(L, cix), &PyList_GET_ITEM(L, cix+1), (size-cix)*sizeof(PyObject*));
+            return v;
+        }
+    }
+#endif
+    return __Pyx__PyObject_PopIndex(L, ix);
+}
+
 
 /////////////// dict_getitem_default.proto ///////////////
 

tests/run/list_pop.pyx

@@ -108,6 +108,10 @@ def index_pop_typed(list L, int i):
     Traceback (most recent call last):
     IndexError: pop index out of range
 
+    >>> index_pop_typed(None, 0)
+    Traceback (most recent call last):
+    AttributeError: 'NoneType' object has no attribute 'pop'
+
     >>> while L:
     ...    _ = index_pop_typed(L, 0)
 
@@ -120,6 +124,48 @@ def index_pop_typed(list L, int i):
     """
     return L.pop(i)
 
+
+@cython.test_assert_path_exists('//PythonCapiCallNode')
+@cython.test_fail_if_path_exists('//SimpleCallNode/AttributeNode')
+def index_pop_list_object_index(list L, i):
+    """
+    >>> L = list(range(10))
+    >>> index_pop_list_object_index(L, 2)
+    2
+    >>> index_pop_list_object_index(L, -2)
+    8
+    >>> L
+    [0, 1, 3, 4, 5, 6, 7, 9]
+    >>> index_pop_list_object_index(L, 100)
+    Traceback (most recent call last):
+    IndexError: pop index out of range
+    >>> index_pop_list_object_index(L, -100)
+    Traceback (most recent call last):
+    IndexError: pop index out of range
+
+    >>> index_pop_list_object_index(None, 0)
+    Traceback (most recent call last):
+    AttributeError: 'NoneType' object has no attribute 'pop'
+    >>> index_pop_list_object_index([1], None)    # doctest: +ELLIPSIS
+    Traceback (most recent call last):
+    TypeError: ...
+    >>> index_pop_list_object_index([1], 'abc')   # doctest: +ELLIPSIS
+    Traceback (most recent call last):
+    TypeError: ...
+
+    >>> while L:
+    ...    _ = index_pop_list_object_index(L, 0)
+
+    >>> L
+    []
+
+    >>> index_pop_list_object_index(L, 0)
+    Traceback (most recent call last):
+    IndexError: pop from empty list
+    """
+    return L.pop(i)
+
+
 @cython.test_assert_path_exists('//PythonCapiCallNode')
 @cython.test_fail_if_path_exists('//SimpleCallNode/AttributeNode')
 def index_pop_literal(list L):