On Tue, 3 May 2016 09:23:29 +0100 Tom Hacohen <[email protected]> said:
> On 29/04/16 18:11, Carsten Haitzler wrote: > > On Fri, 29 Apr 2016 05:57:51 -0700 Tom Hacohen <[email protected]> said: > > > >> tasn pushed a commit to branch master. > >> > >> http://git.enlightenment.org/core/efl.git/commit/?id=9195d008dad901645bebfc83e240649c6b4c69e0 > >> > >> commit 9195d008dad901645bebfc83e240649c6b4c69e0 > >> Author: Tom Hacohen <[email protected]> > >> Date: Fri Apr 29 13:13:00 2016 +0100 > >> > >> Eo keyed data: No need to register to see if a subobject was deleted. > >> > >> This is completely unnecessary. We are holding a reference to the > >> object, it can't get deleted under our feet. > > > > WRONG. > > > > eo_key_obj_set(a, "key", b); > > eo_unref(b); // release last ref to b so the key is the only owner > > // ... some time later > > eo_del(eo_key_obj_get(a, "key")); > > > > perfectly valid. i can unref/del an obj handle i fetched. it may be > > unwise/incorrect, but i COULD have coe that does the above. yes i SHOULD do: > > > > eo_key_obj_del(a, "key"); > > > > instead but it CAN happen like above and the code was clearly written to be > > robust for such cases. you have made code more fragile. > > > > because of the other commits now i can't revert without a whole bunch of > > conflicts. please don't make code more fragile. yes i know eoid makes the eo > > handle robust, but there still is a CHANCE of it being a misdirect (be a > > valid eoid pointing now to the wrong object when gen count is recycled AND > > the eoid table entry is re-used and it happens to get gen count to match). > > the code handled this case and would ensure there is NOT a clash. now it > > can happen thus is more fragile. > > > > I can put it back in if you want, the problem is with this approach you > mentioned here, like many other approaches is that you are *again* > fucking with correct lifetime handling. i said it was incorrect - but it's valid code. just like we have eoid's to deal with bad lifespan handling and mistakes... that's why i had it listen to deletes. in case this happens. > The code you showed above is WRONG/INCORRECT/BAD/BAD/BAD/BAD. The object > a is holding a ref to b (btw, weakref would have made much more sense in > this case and would have solved this discussion without even starting), > while a is holding that ref, you unreffing it beneath its feet is wrong > wrong wrong. I don't know what should be the correct behaviour here with > refs (again, weakrefs would have made more sense!), but it's definitely > not what is there now given your example. > > -- > Tom. > > >> --- > >> src/lib/eo/eo_base_class.c | 50 ++++ > >> +----------------------------------------- 1 file changed, 5 insertions(+), > >> 45 deletions(-) > >> > >> diff --git a/src/lib/eo/eo_base_class.c b/src/lib/eo/eo_base_class.c > >> index 85ed41b..75967bf 100644 > >> --- a/src/lib/eo/eo_base_class.c > >> +++ b/src/lib/eo/eo_base_class.c > >> @@ -95,36 +95,8 @@ _eo_generic_data_node_free(Eo_Generic_Data_Node *node) > >> free(node); > >> } > >> > >> -static Eina_Bool > >> -_eo_base_cb_key_obj_del(void *data, const Eo_Event *event) > >> -{ > >> - Eo *parent_obj = data; > >> - Eo_Base_Data *pd = eo_data_scope_get(parent_obj, EO_BASE_CLASS); > >> - > >> - if (pd) > >> - { > >> - Eo_Generic_Data_Node *node; > >> - Eo_Base_Extension *ext = pd->ext; > >> - > >> - if (!ext) return EINA_TRUE; > >> - EINA_INLIST_FOREACH(ext->generic_data, node) > >> - { > >> - if ((node->d_type == DATA_OBJ) && (node->d.obj == > >> event->obj)) > >> - { > >> - ext->generic_data = eina_inlist_remove > >> - (ext->generic_data, EINA_INLIST_GET(node)); > >> - eo_event_callback_del(node->d.obj, EO_BASE_EVENT_DEL, > >> - _eo_base_cb_key_obj_del, data); > >> - _eo_generic_data_node_free(node); > >> - break; > >> - } > >> - } > >> - } > >> - return EINA_TRUE; > >> -} > >> - > >> static void > >> -_eo_generic_data_del_all(Eo *obj, Eo_Base_Data *pd) > >> +_eo_generic_data_del_all(Eo *obj EINA_UNUSED, Eo_Base_Data *pd) > >> { > >> Eo_Generic_Data_Node *node; > >> Eo_Base_Extension *ext = pd->ext; > >> @@ -137,8 +109,6 @@ _eo_generic_data_del_all(Eo *obj, Eo_Base_Data *pd) > >> EINA_INLIST_GET(node)); > >> if (node->d_type == DATA_OBJ) > >> { > >> - eo_event_callback_del(node->d.obj, EO_BASE_EVENT_DEL, > >> - _eo_base_cb_key_obj_del, obj); > >> eo_unref(node->d.obj); > >> } > >> else if (node->d_type == DATA_VAL) eina_value_free(node->d.val); > >> @@ -147,7 +117,7 @@ _eo_generic_data_del_all(Eo *obj, Eo_Base_Data *pd) > >> } > >> > >> EOLIAN static void > >> -_eo_base_key_data_set(Eo *obj, Eo_Base_Data *pd, const char *key, const > >> void *data) +_eo_base_key_data_set(Eo *obj EINA_UNUSED, Eo_Base_Data *pd, > >> const char *key, const void *data) { > >> Eo_Generic_Data_Node *node; > >> Eo_Base_Extension *ext = pd->ext; > >> @@ -165,8 +135,6 @@ _eo_base_key_data_set(Eo *obj, Eo_Base_Data *pd, const > >> char *key, const void *da (ext->generic_data, EINA_INLIST_GET(node)); > >> if (node->d_type == DATA_OBJ) > >> { > >> - eo_event_callback_del(node->d.obj, > >> EO_BASE_EVENT_DEL, > >> - _eo_base_cb_key_obj_del, > >> obj); eo_unref(node->d.obj); > >> } > >> else if (node->d_type == DATA_VAL) eina_value_free > >> (node->d.val); @@ -220,7 +188,7 @@ _eo_base_key_data_get(const Eo *obj, > >> Eo_Base_Data *pd, const char *key) } > >> > >> EOLIAN static void > >> -_eo_base_key_del(Eo *obj, Eo_Base_Data *pd, const char *key) > >> +_eo_base_key_del(Eo *obj EINA_UNUSED, Eo_Base_Data *pd, const char *key) > >> { > >> Eo_Generic_Data_Node *node; > >> Eo_Base_Extension *ext = pd->ext; > >> @@ -235,8 +203,6 @@ _eo_base_key_del(Eo *obj, Eo_Base_Data *pd, const char > >> *key) (ext->generic_data, EINA_INLIST_GET(node)); > >> if (node->d_type == DATA_OBJ) > >> { > >> - eo_event_callback_del(node->d.obj, EO_BASE_EVENT_DEL, > >> - _eo_base_cb_key_obj_del, obj); > >> eo_unref(node->d.obj); > >> } > >> else if (node->d_type == DATA_VAL) eina_value_free > >> (node->d.val); @@ -247,7 +213,7 @@ _eo_base_key_del(Eo *obj, Eo_Base_Data > >> *pd, const char *key) } > >> > >> EOLIAN static void > >> -_eo_base_key_obj_set(Eo *obj, Eo_Base_Data *pd, const char *key, Eo > >> *objdata) +_eo_base_key_obj_set(Eo *obj EINA_UNUSED, Eo_Base_Data *pd, > >> const char *key, Eo *objdata) { > >> Eo_Generic_Data_Node *node; > >> Eo_Base_Extension *ext = pd->ext; > >> @@ -264,8 +230,6 @@ _eo_base_key_obj_set(Eo *obj, Eo_Base_Data *pd, const > >> char *key, Eo *objdata) (ext->generic_data, EINA_INLIST_GET(node)); > >> if (node->d_type == DATA_OBJ) > >> { > >> - eo_event_callback_del(node->d.obj, > >> EO_BASE_EVENT_DEL, > >> - _eo_base_cb_key_obj_del, > >> obj); eo_unref(node->d.obj); > >> } > >> else if (node->d_type == DATA_VAL) eina_value_free > >> (node->d.val); @@ -284,8 +248,6 @@ _eo_base_key_obj_set(Eo *obj, > >> Eo_Base_Data *pd, const char *key, Eo *objdata) node->key = > >> eina_stringshare_add(key); node->d.obj = objdata; > >> node->d_type = DATA_OBJ; > >> - eo_event_callback_add(node->d.obj, EO_BASE_EVENT_DEL, > >> - _eo_base_cb_key_obj_del, obj); > >> eo_ref(node->d.obj); > >> ext->generic_data = eina_inlist_prepend > >> (ext->generic_data, EINA_INLIST_GET(node)); > >> @@ -322,7 +284,7 @@ _eo_base_key_obj_get(const Eo *obj, Eo_Base_Data *pd, > >> const char *key) } > >> > >> EOLIAN static void > >> -_eo_base_key_value_set(Eo *obj, Eo_Base_Data *pd, const char *key, > >> Eina_Value *value) +_eo_base_key_value_set(Eo *obj EINA_UNUSED, > >> Eo_Base_Data *pd, const char *key, Eina_Value *value) { > >> Eo_Generic_Data_Node *node; > >> Eo_Base_Extension *ext = pd->ext; > >> @@ -339,8 +301,6 @@ _eo_base_key_value_set(Eo *obj, Eo_Base_Data *pd, const > >> char *key, Eina_Value *v (ext->generic_data, EINA_INLIST_GET(node)); > >> if (node->d_type == DATA_OBJ) > >> { > >> - eo_event_callback_del(node->d.obj, > >> EO_BASE_EVENT_DEL, > >> - _eo_base_cb_key_obj_del, > >> obj); eo_unref(node->d.obj); > >> } > >> else if (node->d_type == DATA_VAL) eina_value_free > >> (node->d.val); > >> > >> -- > >> > >> > > > > > > > ------------------------------------------------------------------------------ > Find and fix application performance issues faster with Applications Manager > Applications Manager provides deep performance insights into multiple tiers of > your business applications. It resolves application problems quickly and > reduces your MTTR. Get your free trial! > https://ad.doubleclick.net/ddm/clk/302982198;130105516;z > _______________________________________________ > enlightenment-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) [email protected] ------------------------------------------------------------------------------ Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
