py: Make mp_map_lookup not allocate memory on removal.

dpgeorge · dpgeorge · commit d0e824387e9b · 2014-04-05T23:33:12.000+01:00
diff --git a/py/map.c b/py/map.c
@@ -91,6 +91,12 @@ STATIC void mp_map_rehash(mp_map_t *map) {
     m_del(mp_map_elem_t, old_table, old_alloc);
 }
 
+// MP_MAP_LOOKUP behaviour:
+//  - returns NULL if not found, else the slot it was found in with key,value non-null
+// MP_MAP_LOOKUP_ADD_IF_NOT_FOUND behaviour:
+//  - returns slot, with key non-null and value=MP_OBJ_NULL if it was added
+// MP_MAP_LOOKUP_REMOVE_IF_FOUND behaviour:
+//  - returns NULL if not found, else the slot if was found in with key null and value non-null
 mp_map_elem_t* mp_map_lookup(mp_map_t *map, mp_obj_t index, mp_map_lookup_kind_t lookup_kind) {
     // if the map is a fixed array then we must do a brute force linear search
     if (map->table_is_fixed_array) {
@@ -135,7 +141,7 @@ mp_map_elem_t* mp_map_lookup(mp_map_t *map, mp_obj_t index, mp_map_lookup_kind_t
                 }
                 return slot;
             } else {
-                return MP_OBJ_NULL;
+                return NULL;
             }
         } else if (slot->key == MP_OBJ_SENTINEL) {
             // found deleted slot, remember for later
@@ -146,10 +152,6 @@ mp_map_elem_t* mp_map_lookup(mp_map_t *map, mp_obj_t index, mp_map_lookup_kind_t
             // found index
             // Note: CPython does not replace the index; try x={True:'true'};x[1]='one';x
             if (lookup_kind & MP_MAP_LOOKUP_REMOVE_IF_FOUND) {
-                // this leaks this memory (but see dict_get_helper)
-                mp_map_elem_t *retval = m_new(mp_map_elem_t, 1);
-                retval->key = slot->key;
-                retval->value = slot->value;
                 // delete element in this slot
                 map->used--;
                 if (map->table[(pos + 1) % map->alloc].key == MP_OBJ_NULL) {
@@ -158,7 +160,7 @@ mp_map_elem_t* mp_map_lookup(mp_map_t *map, mp_obj_t index, mp_map_lookup_kind_t
                 } else {
                     slot->key = MP_OBJ_SENTINEL;
                 }
-                return retval;
+                // keep slot->value so that caller can access it if needed
             }
             return slot;
         }
@@ -185,7 +187,7 @@ mp_map_elem_t* mp_map_lookup(mp_map_t *map, mp_obj_t index, mp_map_lookup_kind_t
                     start_pos = pos = hash % map->alloc;
                 }
             } else {
-                return MP_OBJ_NULL;
+                return NULL;
             }
         }
     }
diff --git a/py/objdict.c b/py/objdict.c
@@ -208,10 +208,6 @@ STATIC mp_obj_t dict_get_helper(mp_map_t *self, mp_obj_t key, mp_obj_t deflt, mp
         }
     } else {
         value = elem->value;
-        if (lookup_kind == MP_MAP_LOOKUP_REMOVE_IF_FOUND) {
-            // catch the leak (from mp_map_lookup)
-            m_free(elem, sizeof(mp_map_elem_t));
-        }
     }
     if (lookup_kind == MP_MAP_LOOKUP_ADD_IF_NOT_FOUND) {
         elem->value = value;

Original file line number	Diff line number	Diff line change
`@@ -91,6 +91,12 @@ STATIC void mp_map_rehash(mp_map_t *map) {`
`91`	`91`	`m_del(mp_map_elem_t, old_table, old_alloc);`
`92`	`92`	`}`
`93`	`93`
	`94`	`+// MP_MAP_LOOKUP behaviour:`
	`95`	`+// - returns NULL if not found, else the slot it was found in with key,value non-null`
	`96`	`+// MP_MAP_LOOKUP_ADD_IF_NOT_FOUND behaviour:`
	`97`	`+// - returns slot, with key non-null and value=MP_OBJ_NULL if it was added`
	`98`	`+// MP_MAP_LOOKUP_REMOVE_IF_FOUND behaviour:`
	`99`	`+// - returns NULL if not found, else the slot if was found in with key null and value non-null`
`94`	`100`	`mp_map_elem_t* mp_map_lookup(mp_map_t *map, mp_obj_t index, mp_map_lookup_kind_t lookup_kind) {`
`95`	`101`	`// if the map is a fixed array then we must do a brute force linear search`
`96`	`102`	`if (map->table_is_fixed_array) {`
`@@ -135,7 +141,7 @@ mp_map_elem_t* mp_map_lookup(mp_map_t *map, mp_obj_t index, mp_map_lookup_kind_t`
`135`	`141`	`}`
`136`	`142`	`return slot;`
`137`	`143`	`} else {`
`138`		`- return MP_OBJ_NULL;`
	`144`	`+ return NULL;`
`139`	`145`	`}`
`140`	`146`	`} else if (slot->key == MP_OBJ_SENTINEL) {`
`141`	`147`	`// found deleted slot, remember for later`
`@@ -146,10 +152,6 @@ mp_map_elem_t* mp_map_lookup(mp_map_t *map, mp_obj_t index, mp_map_lookup_kind_t`
`146`	`152`	`// found index`
`147`	`153`	`// Note: CPython does not replace the index; try x={True:'true'};x[1]='one';x`
`148`	`154`	`if (lookup_kind & MP_MAP_LOOKUP_REMOVE_IF_FOUND) {`
`149`		`- // this leaks this memory (but see dict_get_helper)`
`150`		`- mp_map_elem_t *retval = m_new(mp_map_elem_t, 1);`
`151`		`- retval->key = slot->key;`
`152`		`- retval->value = slot->value;`
`153`	`155`	`// delete element in this slot`
`154`	`156`	`map->used--;`
`155`	`157`	`if (map->table[(pos + 1) % map->alloc].key == MP_OBJ_NULL) {`
`@@ -158,7 +160,7 @@ mp_map_elem_t* mp_map_lookup(mp_map_t *map, mp_obj_t index, mp_map_lookup_kind_t`
`158`	`160`	`} else {`
`159`	`161`	`slot->key = MP_OBJ_SENTINEL;`
`160`	`162`	`}`
`161`		`- return retval;`
	`163`	`+ // keep slot->value so that caller can access it if needed`
`162`	`164`	`}`
`163`	`165`	`return slot;`
`164`	`166`	`}`
`@@ -185,7 +187,7 @@ mp_map_elem_t* mp_map_lookup(mp_map_t *map, mp_obj_t index, mp_map_lookup_kind_t`
`185`	`187`	`start_pos = pos = hash % map->alloc;`
`186`	`188`	`}`
`187`	`189`	`} else {`
`188`		`- return MP_OBJ_NULL;`
	`190`	`+ return NULL;`
`189`	`191`	`}`
`190`	`192`	`}`
`191`	`193`	`}`
Original file line number	Diff line number	Diff line change
`@@ -208,10 +208,6 @@ STATIC mp_obj_t dict_get_helper(mp_map_t *self, mp_obj_t key, mp_obj_t deflt, mp`
`208`	`208`	`}`
`209`	`209`	`} else {`
`210`	`210`	`value = elem->value;`
`211`		`- if (lookup_kind == MP_MAP_LOOKUP_REMOVE_IF_FOUND) {`
`212`		`- // catch the leak (from mp_map_lookup)`
`213`		`- m_free(elem, sizeof(mp_map_elem_t));`
`214`		`- }`
`215`	`211`	`}`
`216`	`212`	`if (lookup_kind == MP_MAP_LOOKUP_ADD_IF_NOT_FOUND) {`
`217`	`213`	`elem->value = value;`