Merge pull request #1310 from Daetalus/list_cpython

Switch to CPython list sort(Not its list implementation)

from_cpython/CMakeLists.txt

@@ -96,6 +96,7 @@ file(GLOB_RECURSE STDOBJECT_SRCS Objects
     fileobject.c
     import.c
     iterobject.c
+    listobject.c
     memoryobject.c
     obmalloc.c
     sliceobject.c

from_cpython/Lib/test/test_sort.py

-# expected: fail
 from test import test_support
 import random
 import sys

src/runtime/builtin_modules/builtins.cpp

@@ -452,17 +452,44 @@ Box* notimplementedRepr(Box* self) {
     return boxString("NotImplemented");
 }
 
-Box* sorted(Box* obj, Box* cmp, Box* key, Box** args) {
-    Box* reverse = args[0];
+// Copied from CPython with some minor modifications
+static PyObject* builtin_sorted(PyObject* self, PyObject* args, PyObject* kwds) {
+    PyObject* newlist, *v, *seq, * compare = NULL, * keyfunc = NULL, *newargs;
+    PyObject* callable;
+    static const char* kwlist[] = { "iterable", "cmp", "key", "reverse", 0 };
+    int reverse;
 
-    BoxedList* rtn = new BoxedList();
-    AUTO_DECREF(rtn);
-    for (Box* e : obj->pyElements()) {
-        listAppendInternalStolen(rtn, e);
+    /* args 1-4 should match listsort in Objects/listobject.c */
+    if (!PyArg_ParseTupleAndKeywords(args, kwds, "O|OOi:sorted", const_cast<char**>(kwlist), &seq, &compare, &keyfunc,
+                                     &reverse))
+        return NULL;
+
+    newlist = PySequence_List(seq);
+    if (newlist == NULL)
+        return NULL;
+
+    callable = PyObject_GetAttrString(newlist, "sort");
+    if (callable == NULL) {
+        Py_DECREF(newlist);
+        return NULL;
     }
 
-    listSort(rtn, cmp, key, reverse);
-    return incref(rtn);
+    newargs = PyTuple_GetSlice(args, 1, 4);
+    if (newargs == NULL) {
+        Py_DECREF(newlist);
+        Py_DECREF(callable);
+        return NULL;
+    }
+
+    v = PyObject_Call(callable, newargs, kwds);
+    Py_DECREF(newargs);
+    Py_DECREF(callable);
+    if (v == NULL) {
+        Py_DECREF(newlist);
+        return NULL;
+    }
+    Py_DECREF(v);
+    return newlist;
 }
 
 Box* isinstance_func(Box* obj, Box* cls) {
@@ -2487,13 +2514,6 @@ void setupBuiltins() {
     enumerate_cls->tp_iter = PyObject_SelfIter;
     builtins_module->giveAttrBorrowed("enumerate", enumerate_cls);
 
-
-    FunctionMetadata* sorted_func
-        = new FunctionMetadata(4, false, false, ParamNames({ "", "cmp", "key", "reverse" }, "", ""));
-    sorted_func->addVersion((void*)sorted, LIST, { UNKNOWN, UNKNOWN, UNKNOWN, UNKNOWN });
-    builtins_module->giveAttr("sorted", new BoxedBuiltinFunctionOrMethod(
-                                            sorted_func, "sorted", { Py_None, Py_None, Py_False }, NULL, sorted_doc));
-
     builtins_module->giveAttrBorrowed("True", Py_True);
     builtins_module->giveAttrBorrowed("False", Py_False);
 
@@ -2611,6 +2631,7 @@ void setupBuiltins() {
         { "oct", builtin_oct, METH_O, oct_doc },
         { "print", (PyCFunction)builtin_print, METH_VARARGS | METH_KEYWORDS, print_doc },
         { "reload", builtin_reload, METH_O, reload_doc },
+        { "sorted", (PyCFunction)builtin_sorted, METH_VARARGS | METH_KEYWORDS, sorted_doc },
     };
     for (auto& md : builtin_methods) {
         builtins_module->giveAttr(md.ml_name, new BoxedCApiFunction(&md, NULL, autoDecref(boxString("__builtin__"))));

src/runtime/inline/list.cpp

@@ -131,15 +131,15 @@ const int BoxedList::INITIAL_CAPACITY = 8;
 // TODO the inliner doesn't want to inline these; is there any point to having them in the inline section?
 void BoxedList::shrink() {
     // TODO more attention to the shrink condition to avoid frequent shrink and alloc
-    if (capacity > size * 3) {
-        int new_capacity = std::max(static_cast<int64_t>(INITIAL_CAPACITY), capacity / 2);
+    if (allocated > size * 3) {
+        int new_allocated = std::max(static_cast<int64_t>(INITIAL_CAPACITY), allocated / 2);
         if (size > 0) {
-            elts = GCdArray::grow(elts, new_capacity);
-            capacity = new_capacity;
+            elts = GCdArray::grow(elts, new_allocated);
+            allocated = new_allocated;
         } else if (size == 0) {
             delete elts;
             elts = NULL;
-            capacity = 0;
+            allocated = 0;
         }
     }
 }
@@ -149,14 +149,14 @@ extern "C" void listAppendArrayInternal(Box* s, Box** v, int nelts) {
     assert(PyList_Check(s));
     BoxedList* self = static_cast<BoxedList*>(s);
 
-    assert(self->size <= self->capacity);
+    assert(self->size <= self->allocated);
     self->ensure(nelts);
 
     for (int i = 0; i < nelts; i++) {
         Py_INCREF(v[i]);
     }
 
-    assert(self->size <= self->capacity);
+    assert(self->size <= self->allocated);
     memcpy(&self->elts->elts[self->size], &v[0], nelts * sizeof(Box*));
 
     self->size += nelts;

src/runtime/inline/list.h

@@ -22,23 +22,23 @@ namespace pyston {
 
 // TODO the inliner doesn't want to inline these; is there any point to having them in the inline section?
 inline void BoxedList::grow(int min_free) {
-    if (capacity == 0) {
+    if (allocated == 0) {
         const int INITIAL_CAPACITY = 8;
         int initial = std::max(INITIAL_CAPACITY, min_free);
         elts = new (initial) GCdArray();
-        capacity = initial;
+        allocated = initial;
     } else {
-        int new_capacity = std::max(capacity * 2, size + min_free);
-        elts = GCdArray::grow(elts, new_capacity);
-        capacity = new_capacity;
+        int new_allocated = std::max(allocated * 2, size + min_free);
+        elts = GCdArray::grow(elts, new_allocated);
+        allocated = new_allocated;
     }
 }
 
 inline void BoxedList::ensure(int min_free) {
-    if (unlikely(size + min_free > capacity)) {
+    if (unlikely(size + min_free > allocated)) {
         grow(min_free);
     }
-    assert(capacity >= size + min_free);
+    assert(allocated >= size + min_free);
 }
 
 // TODO the inliner doesn't want to inline these; is there any point to having them in the inline section?
@@ -48,10 +48,10 @@ extern "C" inline void listAppendInternalStolen(Box* s, Box* v) {
     assert(PyList_Check(s));
     BoxedList* self = static_cast<BoxedList*>(s);
 
-    assert(self->size <= self->capacity);
+    assert(self->size <= self->allocated || self->allocated == -1);
     self->ensure(1);
 
-    assert(self->size < self->capacity);
+    assert(self->size < self->allocated);
     self->elts->elts[self->size] = v;
     self->size++;
 }

src/runtime/list.cpp

@@ -17,6 +17,7 @@
 #include <algorithm>
 #include <cstring>
 
+#include "capi/typeobject.h"
 #include "capi/types.h"
 #include "core/ast.h"
 #include "core/common.h"
@@ -27,6 +28,8 @@
 #include "runtime/types.h"
 #include "runtime/util.h"
 
+extern "C" PyObject* listsort(PyListObject* self, PyObject* args, PyObject* kwds) noexcept;
+
 namespace pyston {
 
 static int list_ass_slice(PyListObject* a, Py_ssize_t ilow, Py_ssize_t ihigh, PyObject* v);
@@ -991,64 +994,6 @@ void _sortArray(Box** elts, long num_elts, Box* cmp, Box* key) {
     }
 }
 
-void listSort(BoxedList* self, Box* cmp, Box* key, Box* reverse) {
-    assert(PyList_Check(self));
-
-    if (cmp == Py_None)
-        cmp = NULL;
-
-    if (key == Py_None)
-        key = NULL;
-
-    RELEASE_ASSERT(!cmp || !key, "Specifying both the 'cmp' and 'key' keywords is currently not supported");
-
-    auto orig_size = self->size;
-    auto orig_elts = self->elts;
-
-    self->elts = new (0) GCdArray();
-    self->size = 0;
-
-    try {
-        _sortArray(orig_elts->elts, orig_size, cmp, key);
-    } catch (ExcInfo e) {
-        delete self->elts;
-        self->elts = orig_elts;
-        self->size = orig_size;
-        throw e;
-    }
-
-    delete self->elts;
-    self->elts = orig_elts;
-    self->size = orig_size;
-
-    if (nonzero(reverse)) {
-        Box* r = listReverse(self);
-        Py_DECREF(r);
-    }
-}
-
-Box* listSortFunc(BoxedList* self, Box* cmp, Box* key, Box** _args) {
-    Box* reverse = _args[0];
-    listSort(self, cmp, key, reverse);
-    Py_RETURN_NONE;
-}
-
-extern "C" int PyList_Sort(PyObject* v) noexcept {
-    if (v == NULL || !PyList_Check(v)) {
-        PyErr_BadInternalCall();
-        return -1;
-    }
-
-    try {
-        listSort((BoxedList*)v, Py_None, Py_None, Py_False);
-    } catch (ExcInfo e) {
-        setCAPIException(e);
-        return -1;
-    }
-
-    return 0;
-}
-
 extern "C" Box* PyList_GetSlice(PyObject* a, Py_ssize_t ilow, Py_ssize_t ihigh) noexcept {
     assert(PyList_Check(a));
     BoxedList* self = static_cast<BoxedList*>(a);
@@ -1481,7 +1426,7 @@ int BoxedList::clear(Box* _a) noexcept {
            this list, we make it empty first. */
         i = Py_SIZE(a);
         Py_SIZE(a) = 0;
-        a->capacity = 0;
+        a->allocated = 0;
         auto old_elts = a->elts;
         a->elts = NULL;
         while (--i >= 0) {
@@ -1495,6 +1440,12 @@ int BoxedList::clear(Box* _a) noexcept {
     return 0;
 }
 
+PyDoc_STRVAR(sort_doc, "L.sort(cmp=None, key=None, reverse=False) -- stable sort *IN PLACE*;\n\
+cmp(x, y) -> -1, 0, 1");
+
+static PyMethodDef list_methods[]
+    = { { "sort", (PyCFunction)listsort, METH_VARARGS | METH_KEYWORDS, sort_doc }, { NULL, NULL, 0, NULL } };
+
 void setupList() {
     static PySequenceMethods list_as_sequence;
     list_cls->tp_as_sequence = &list_as_sequence;
@@ -1568,10 +1519,6 @@ void setupList() {
     list_cls->giveAttr("__iadd__", new BoxedFunction(FunctionMetadata::create((void*)listIAdd, UNKNOWN, 2)));
     list_cls->giveAttr("__add__", new BoxedFunction(FunctionMetadata::create((void*)listAdd, UNKNOWN, 2)));
 
-    list_cls->giveAttr("sort",
-                       new BoxedFunction(FunctionMetadata::create((void*)listSortFunc, NONE, 4, false, false,
-                                                                  ParamNames({ "", "cmp", "key", "reverse" }, "", "")),
-                                         { Py_None, Py_None, Py_False }));
     list_cls->giveAttr("__contains__", new BoxedFunction(FunctionMetadata::create((void*)listContains, BOXED_BOOL, 2)));
 
     list_cls->giveAttr(
@@ -1585,6 +1532,7 @@ void setupList() {
     list_cls->giveAttr("reverse", new BoxedFunction(FunctionMetadata::create((void*)listReverse, NONE, 1)));
 
     list_cls->giveAttrBorrowed("__hash__", Py_None);
+    add_methods(list_cls, list_methods);
     list_cls->freeze();
     list_cls->tp_iter = listIter;
     list_cls->tp_repr = list_repr;

src/runtime/list.h

@@ -51,7 +51,6 @@ Box* listreviterHasnext(Box* self);
 llvm_compat_bool listreviterHasnextUnboxed(Box* self);
 Box* listreviterNext(Box* self);
 Box* listreviter_next(Box* s) noexcept;
-void listSort(BoxedList* self, Box* cmp, Box* key, Box* reverse);
 extern "C" Box* listAppend(Box* self, Box* v);
 }
 

src/runtime/types.h

@@ -718,9 +718,9 @@ private:
 public:
     Py_ssize_t size;
     GCdArray* elts;
-    Py_ssize_t capacity;
+    Py_ssize_t allocated;
 
-    BoxedList() __attribute__((visibility("default"))) : size(0), elts(NULL), capacity(0) {}
+    BoxedList() __attribute__((visibility("default"))) : size(0), elts(NULL), allocated(0) {}
 
     void ensure(int min_free);
     void shrink();
@@ -737,7 +737,7 @@ static_assert(sizeof(BoxedList) >= sizeof(PyListObject), "");
 static_assert(offsetof(BoxedList, size) == offsetof(PyListObject, ob_size), "");
 static_assert(offsetof(BoxedList, elts) == offsetof(PyListObject, ob_item), "");
 static_assert(offsetof(GCdArray, elts) == 0, "");
-static_assert(offsetof(BoxedList, capacity) == offsetof(PyListObject, allocated), "");
+static_assert(offsetof(BoxedList, allocated) == offsetof(PyListObject, allocated), "");
 
 #define PyTuple_MAXSAVESIZE 20   /* Largest tuple to save on free list */
 #define PyTuple_MAXFREELIST 2000 /* Maximum number of tuples of each size to save */

test/CPYTHON_TEST_NOTES.md

@@ -144,7 +144,6 @@ test_scope              eval of code object from existing function (not currentl
 test_scriptpackages     [unknown]
 test_site               [unknown]
 test_socket             [unknown]
-test_sort               argument specification issue in listSort?
 test_startfile          [unknown]
 test_str                memory leak?
 test_structmembers      [unknown]

test/tests/class_and_func_name.py

@@ -50,10 +50,10 @@ set_name(f, "b\0b")
 #del_name(f)
 set_name(f, "")
 
-print sorted.__name__
+print cmp.__name__
 # should all fail:
-set_name(sorted, "blah")
-set_name(sorted, 5)
+set_name(cmp, "blah")
+set_name(cmp, 5)
 
 import abc
 class D(C):

test/tests/list.py

@@ -279,3 +279,7 @@ try:
     a.index(None)
 except ValueError as e:
     print e
+
+a_list = [(0, 0, 0), (1.0, 2, 0.0), (1.0, 3, 0.0)]
+reverse_list = sorted(a_list, key=lambda x: x[0], reverse=True)
+print(reverse_list)

test/tests/sort_cmp.py

-# expected: fail
-# - we don't support the cmp= keyword yet
-# (apparently it's removed in Python 3)
-
 printed = False
 def mycmp(x, y):
     global printed