From f4c9b33abf0ac6ff97cd39331d125a74fd2bb897 Mon Sep 17 00:00:00 2001
From: Damien George <damien.p.george@gmail.com>
Date: Tue, 8 Apr 2014 21:32:29 +0100
Subject: [PATCH] py: Remove DELETE_SUBSCR opcode, combine with STORE_SUBSCR.

This makes the runtime and object APIs more consistent.  mp_store_subscr
functionality now moved into objects (ie list and dict store_item).
---
 py/bc0.h                   |  1 -
 py/emitbc.c                |  6 ++++--
 py/obj.h                   |  9 +++++----
 py/objarray.c              |  4 ++++
 py/objdict.c               |  9 +++++++++
 py/objlist.c               | 12 ++++++++++++
 py/objtype.c               |  5 +++++
 py/runtime.c               | 37 +++++++++----------------------------
 py/vm.c                    |  5 -----
 tests/basics/del-attr.py   | 28 ++++++++++++++++++++++++++++
 tests/basics/del-name.py   | 18 ++++++++++++++++++
 tests/basics/del-subscr.py | 13 +++++++++++++
 12 files changed, 107 insertions(+), 40 deletions(-)
 create mode 100644 tests/basics/del-attr.py
 create mode 100644 tests/basics/del-name.py
 create mode 100644 tests/basics/del-subscr.py

diff --git a/py/bc0.h b/py/bc0.h
index 0fe33d48a..83993f899 100644
--- a/py/bc0.h
+++ b/py/bc0.h
@@ -40,7 +40,6 @@
 
 #define MP_BC_DELETE_NAME        (0x39) // qstr
 #define MP_BC_DELETE_GLOBAL      (0x3a) // qstr
-#define MP_BC_DELETE_SUBSCR      (0x3b)
 
 #define MP_BC_DUP_TOP            (0x40)
 #define MP_BC_DUP_TOP_TWO        (0x41)
diff --git a/py/emitbc.c b/py/emitbc.c
index 2a390a056..50bb0016e 100644
--- a/py/emitbc.c
+++ b/py/emitbc.c
@@ -37,6 +37,7 @@ struct _emit_t {
 };
 
 STATIC void emit_bc_rot_two(emit_t *emit);
+STATIC void emit_bc_rot_three(emit_t *emit);
 
 emit_t *emit_bc_new(uint max_num_labels) {
     emit_t *emit = m_new0(emit_t, 1);
@@ -515,8 +516,9 @@ STATIC void emit_bc_delete_attr(emit_t *emit, qstr qstr) {
 }
 
 STATIC void emit_bc_delete_subscr(emit_t *emit) {
-    emit_bc_pre(emit, -2);
-    emit_write_byte_code_byte(emit, MP_BC_DELETE_SUBSCR);
+    emit_bc_load_null(emit);
+    emit_bc_rot_three(emit);
+    emit_bc_store_subscr(emit);
 }
 
 STATIC void emit_bc_dup_top(emit_t *emit) {
diff --git a/py/obj.h b/py/obj.h
index 77f7c0b8f..92dd6a8a0 100644
--- a/py/obj.h
+++ b/py/obj.h
@@ -163,8 +163,8 @@ typedef mp_obj_t (*mp_call_fun_t)(mp_obj_t fun, uint n_args, uint n_kw, const mp
 typedef mp_obj_t (*mp_unary_op_fun_t)(int op, mp_obj_t);
 typedef mp_obj_t (*mp_binary_op_fun_t)(int op, mp_obj_t, mp_obj_t);
 typedef void (*mp_load_attr_fun_t)(mp_obj_t self_in, qstr attr, mp_obj_t *dest); // for fail, do nothing; for attr, dest[0] = value; for method, dest[0] = method, dest[1] = self
-typedef bool (*mp_store_attr_fun_t)(mp_obj_t self_in, qstr attr, mp_obj_t value); // return true if store succeeded
-typedef bool (*mp_store_item_fun_t)(mp_obj_t self_in, mp_obj_t index, mp_obj_t value); // return true if store succeeded
+typedef bool (*mp_store_attr_fun_t)(mp_obj_t self_in, qstr attr, mp_obj_t value); // return true if store succeeded; if value==MP_OBJ_NULL then delete
+typedef bool (*mp_store_item_fun_t)(mp_obj_t self_in, mp_obj_t index, mp_obj_t value); // return true if store succeeded; if value==MP_OBJ_NULL then delete
 
 typedef struct _mp_method_t {
     qstr name;
@@ -218,8 +218,9 @@ struct _mp_obj_type_t {
 
     mp_load_attr_fun_t load_attr;
     mp_store_attr_fun_t store_attr; // if value is MP_OBJ_NULL, then delete that attribute
-    // Implements container[index] = val; note that load_item is implemented
-    // by binary_op(RT_BINARY_OP_SUBSCR)
+
+    // Implements container[index] = val.  If val == MP_OBJ_NULL, then it's a delete.
+    // Note that load_item is implemented by binary_op(RT_BINARY_OP_SUBSCR)
     mp_store_item_fun_t store_item;
 
     mp_fun_1_t getiter;
diff --git a/py/objarray.c b/py/objarray.c
index c65673fff..15268112a 100644
--- a/py/objarray.c
+++ b/py/objarray.c
@@ -143,6 +143,10 @@ STATIC mp_obj_t array_append(mp_obj_t self_in, mp_obj_t arg) {
 STATIC MP_DEFINE_CONST_FUN_OBJ_2(array_append_obj, array_append);
 
 STATIC bool array_store_item(mp_obj_t self_in, mp_obj_t index_in, mp_obj_t value) {
+    if (value == MP_OBJ_NULL) {
+        // delete item; does this need to be implemented?
+        return false;
+    }
     mp_obj_array_t *o = self_in;
     uint index = mp_get_index(o->base.type, o->len, index_in, false);
     mp_binary_set_val(o->typecode, o->items, index, value);
diff --git a/py/objdict.c b/py/objdict.c
index 80337198e..85986448c 100644
--- a/py/objdict.c
+++ b/py/objdict.c
@@ -114,6 +114,14 @@ STATIC mp_obj_t dict_binary_op(int op, mp_obj_t lhs_in, mp_obj_t rhs_in) {
     }
 }
 
+STATIC bool dict_store_item(mp_obj_t self_in, mp_obj_t index, mp_obj_t value) {
+    if (value == MP_OBJ_NULL) {
+        mp_obj_dict_delete(self_in, index);
+    } else {
+        mp_obj_dict_store(self_in, index, value);
+    }
+    return true;
+}
 
 /******************************************************************************/
 /* dict iterator                                                              */
@@ -484,6 +492,7 @@ const mp_obj_type_t mp_type_dict = {
     .make_new = dict_make_new,
     .unary_op = dict_unary_op,
     .binary_op = dict_binary_op,
+    .store_item = dict_store_item,
     .getiter = dict_getiter,
     .locals_dict = (mp_obj_t)&dict_locals_dict,
 };
diff --git a/py/objlist.c b/py/objlist.c
index 371d1cb26..a3b7e79ab 100644
--- a/py/objlist.c
+++ b/py/objlist.c
@@ -19,6 +19,7 @@ typedef struct _mp_obj_list_t {
 STATIC mp_obj_t mp_obj_new_list_iterator(mp_obj_list_t *list, int cur);
 STATIC mp_obj_list_t *list_new(uint n);
 STATIC mp_obj_t list_extend(mp_obj_t self_in, mp_obj_t arg_in);
+STATIC mp_obj_t list_pop(uint n_args, const mp_obj_t *args);
 
 // TODO: Move to mpconfig.h
 #define LIST_MIN_ALLOC 4
@@ -146,6 +147,16 @@ STATIC mp_obj_t list_binary_op(int op, mp_obj_t lhs, mp_obj_t rhs) {
     }
 }
 
+STATIC bool list_store_item(mp_obj_t self_in, mp_obj_t index, mp_obj_t value) {
+    if (value == MP_OBJ_NULL) {
+        mp_obj_t args[2] = {self_in, index};
+        list_pop(2, args);
+    } else {
+        mp_obj_list_store(self_in, index, value);
+    }
+    return true;
+}
+
 STATIC mp_obj_t list_getiter(mp_obj_t o_in) {
     return mp_obj_new_list_iterator(o_in, 0);
 }
@@ -349,6 +360,7 @@ const mp_obj_type_t mp_type_list = {
     .make_new = list_make_new,
     .unary_op = list_unary_op,
     .binary_op = list_binary_op,
+    .store_item = list_store_item,
     .getiter = list_getiter,
     .locals_dict = (mp_obj_t)&list_locals_dict,
 };
diff --git a/py/objtype.c b/py/objtype.c
index a2094f671..cb8cfc532 100644
--- a/py/objtype.c
+++ b/py/objtype.c
@@ -269,6 +269,11 @@ STATIC bool class_store_attr(mp_obj_t self_in, qstr attr, mp_obj_t value) {
 }
 
 bool class_store_item(mp_obj_t self_in, mp_obj_t index, mp_obj_t value) {
+    if (value == MP_OBJ_NULL) {
+        // delete item
+        // TODO implement me!
+        return false;
+    }
     mp_obj_class_t *self = self_in;
     mp_obj_t member = mp_obj_class_lookup(self->base.type, MP_QSTR___setitem__);
     if (member != MP_OBJ_NULL) {
diff --git a/py/runtime.c b/py/runtime.c
index 3400d8dc8..ef07e39bf 100644
--- a/py/runtime.c
+++ b/py/runtime.c
@@ -841,37 +841,18 @@ void mp_store_attr(mp_obj_t base, qstr attr, mp_obj_t value) {
 
 void mp_store_subscr(mp_obj_t base, mp_obj_t index, mp_obj_t value) {
     DEBUG_OP_printf("store subscr %p[%p] <- %p\n", base, index, value);
-    if (MP_OBJ_IS_TYPE(base, &mp_type_list)) {
-        // list store
-        mp_obj_list_store(base, index, value);
-    } else if (MP_OBJ_IS_TYPE(base, &mp_type_dict)) {
-        // dict store
-        mp_obj_dict_store(base, index, value);
-    } else {
-        mp_obj_type_t *type = mp_obj_get_type(base);
-        if (type->store_item != NULL) {
-            bool r = type->store_item(base, index, value);
-            if (r) {
-                return;
-            }
-            // TODO: call base classes here?
+    mp_obj_type_t *type = mp_obj_get_type(base);
+    if (type->store_item != NULL) {
+        bool r = type->store_item(base, index, value);
+        if (r) {
+            return;
         }
-        nlr_raise(mp_obj_new_exception_msg_varg(&mp_type_TypeError, "'%s' object does not support item assignment", mp_obj_get_type_str(base)));
+        // TODO: call base classes here?
     }
-}
-
-void mp_delete_subscr(mp_obj_t base, mp_obj_t index) {
-    DEBUG_OP_printf("delete subscr %p[%p]\n", base, index);
-    /* list delete not implemented
-    if (MP_OBJ_IS_TYPE(base, &mp_type_list)) {
-        // list delete
-        mp_obj_list_delete(base, index);
-    } else */
-    if (MP_OBJ_IS_TYPE(base, &mp_type_dict)) {
-        // dict delete
-        mp_obj_dict_delete(base, index);
-    } else {
+    if (value == MP_OBJ_NULL) {
         nlr_raise(mp_obj_new_exception_msg_varg(&mp_type_TypeError, "'%s' object does not support item deletion", mp_obj_get_type_str(base)));
+    } else {
+        nlr_raise(mp_obj_new_exception_msg_varg(&mp_type_TypeError, "'%s' object does not support item assignment", mp_obj_get_type_str(base)));
     }
 }
 
diff --git a/py/vm.c b/py/vm.c
index 474a1aeaf..170104467 100644
--- a/py/vm.c
+++ b/py/vm.c
@@ -324,11 +324,6 @@ dispatch_loop:
                         mp_delete_global(qst);
                         break;
 
-                    case MP_BC_DELETE_SUBSCR:
-                        mp_delete_subscr(sp[-1], sp[0]);
-                        sp -= 2;
-                        break;
-
                     case MP_BC_DUP_TOP:
                         obj1 = TOP();
                         PUSH(obj1);
diff --git a/tests/basics/del-attr.py b/tests/basics/del-attr.py
new file mode 100644
index 000000000..501c729ce
--- /dev/null
+++ b/tests/basics/del-attr.py
@@ -0,0 +1,28 @@
+# del a class attribute
+
+del C.f
+try:
+    print(C.x)
+except AttributeError:
+    print("AttributeError")
+try:
+    del C.f
+except AttributeError:
+    print("AttributeError")
+
+# del an instance attribute
+
+c = C()
+
+c.x = 1
+print(c.x)
+
+del c.x
+try:
+    print(c.x)
+except AttributeError:
+    print("AttributeError")
+try:
+    del c.x
+except AttributeError:
+    print("AttributeError")
diff --git a/tests/basics/del-name.py b/tests/basics/del-name.py
new file mode 100644
index 000000000..f75a2f5dc
--- /dev/null
+++ b/tests/basics/del-name.py
@@ -0,0 +1,18 @@
+# del global
+
+x = 1
+print(x)
+del x
+try:
+    print(x)
+except NameError:
+    print("NameError")
+try:
+    del x
+except: # NameError:
+    # FIXME uPy returns KeyError for this
+    print("NameError")
+
+class C:
+    def f():
+        pass
diff --git a/tests/basics/del-subscr.py b/tests/basics/del-subscr.py
new file mode 100644
index 000000000..67910c623
--- /dev/null
+++ b/tests/basics/del-subscr.py
@@ -0,0 +1,13 @@
+l = [1, 2, 3]
+print(l)
+del l[0]
+print(l)
+del l[-1]
+print(l)
+
+d = {1:2, 3:4, 5:6}
+del d[1]
+del d[3]
+print(d)
+del d[5]
+print(d)
-- 
GitLab