On Thu, 20 Aug 2015 10:22:42 +0300 Daniel Zaoui <daniel.za...@samsung.com> said:
> On Thu, 20 Aug 2015 15:48:23 +0900 > Carsten Haitzler <ras...@rasterman.com> (The Rasterman) wrote: > > > On Thu, 20 Aug 2015 09:09:44 +0300 Daniel Zaoui > > <daniel.za...@samsung.com> said: > > > > > Hi, > > > > > > On Wed, 19 Aug 2015 20:53:53 -0700 > > > Carsten Haitzler <ras...@rasterman.com> wrote: > > > > > > > raster pushed a commit to branch master. > > > > > > > > http://git.enlightenment.org/core/efl.git/commit/?id=8689d54471aafdd7a5b5a27ce116bf2ab68c1042 > > > > > > > > commit 8689d54471aafdd7a5b5a27ce116bf2ab68c1042 > > > > Author: Carsten Haitzler (Rasterman) <ras...@rasterman.com> > > > > Date: Thu Aug 20 12:50:52 2015 +0900 > > > > > > > > eo - destruction - ensure child is removed from parent child > > > > list > > > > this follows on from cbc1a217bfc8b5c6dd94f0448f19245c43eb05e0 > > > > as this code was correct, but was then causing bugs due to > > > > children staying in their parent lists. this should never have > > > > happened and this is really bad. this fixes this and ensures > > > > children on destruction are gone from their parent lists. > > > > > > > > @fix > > > > --- > > > > src/lib/eo/eo_base_class.c | 14 ++++++++++---- > > > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/src/lib/eo/eo_base_class.c > > > > b/src/lib/eo/eo_base_class.c index fe52203..9f8252b 100644 > > > > --- a/src/lib/eo/eo_base_class.c > > > > +++ b/src/lib/eo/eo_base_class.c > > > > @@ -977,7 +977,6 @@ EOLIAN static void > > > > _eo_base_destructor(Eo *obj, Eo_Base_Data *pd) > > > > { > > > > Eo *child; > > > > - Eo_Base_Data *child_pd; > > > > > > > > DBG("%p - %s.", obj, eo_class_name_get(MY_CLASS)); > > > > > > > > @@ -987,11 +986,18 @@ _eo_base_destructor(Eo *obj, Eo_Base_Data > > > > *pd) while (pd->children) > > > > { > > > > child = eina_list_data_get(pd->children); > > > > - child_pd = eo_data_scope_get(child, EO_BASE_CLASS); > > > > - pd->children = eina_list_remove_list(pd->children, > > > > pd->children); > > > > - child_pd->parent_list = NULL; > > > > eo_do(child, eo_parent_set(NULL)); > > > > } > > > > + // remove child from its parent on destruction - ha to be done > > > > + if (pd->parent) > > > > + { > > > > + Eo_Base_Data *parent_pd; > > > > + > > > > + parent_pd = eo_data_scope_get(pd->parent, EO_BASE_CLASS); > > > > + parent_pd->children = > > > > eina_list_remove_list(parent_pd->children, > > > > + > > > > pd->parent_list); > > > > + pd->parent_list = NULL; > > > > + } > > > > > > > > _eo_generic_data_del_all(pd); > > > > _wref_destruct(pd); > > > > > > > > > > The parent should never be !NULL when reaching the destructor. Imo, > > > this code has not to be here. Instead, an error message should be > > > displayed in the case the parent is still connected to the object. > > > There is a bug but definitely the solution doesn't have to be here. > > > I think this issue may happen if eo_del is never called and > > > eo_unref is called instead. We need to check inside _eo_unref that > > > the parent is NULL and display an error message. > > > > > > Tom, any thoughts? > > > > eo_suite simple examples show this case. in fact i think the test > > case is broken too... but as such i followed the code, and read it. > > if you destruct a child, the parent still has a list entry pointing > > to it. > > > > _eo_unref() gets to 0, and _eo_del_internal() calls the destructor... > > eo_del unparents it and that's the issue of the test, as it is not called an eo_unref to a refcount of 0 should be the same as del in this case. it causes destruction. the difference is a del is marking the object as "i want to delete this and any references now are just due to people holding on to them". at least conceptually. del and unref are really equivalent other than the above. > before destruction. I think the test has not been adapted when references > mechanism has changed. Anyway, eo_unref should display an error message > indicating that only one ref exists and belongs to the parent. I think it > should force unparenting too. or .. just have the destructor do it as i did - in one place only. :) > > this does nothing in the way of removing from a parent (read it). the > > base class destructor is the only place to do this... and so i added > > it. > > > > look at > > > > START_TEST(eo_refs) > > > > here: > > > > obj = eo_add(SIMPLE_CLASS, NULL); > > obj2 = eo_add(SIMPLE_CLASS, obj); > > eo_unref(obj2); > > eo_ref(obj2); > > eo_del(obj2); > > eo_unref(obj); > > I don't see how this test is supposed to work. eo_ref and eo_del should not > work well as the object is already deleted. As I said, it doesn't seem > updated with last Eo changes. the ref of obj2 after the unref should absolutely not work. it should fail. the test doesnt test for this being the case so i have no idea what the intent was. but this did bring up the point that an unref and a del should be equivalent. :) > > > > we create obj, then obj2 as a child of obj, then unref obj2. after > > this unref of obj2, obj2 was still in the child list of obj. this > > isn't a complex case. its an insanely simple one. our own test cases > > never caught this issue until i "fixed" the code above that pointed > > this out. the base class destructor never removes an object from its > > parent list. no code in the destructor to do that at all. i fixed > > that :) > > The code is not supposed to be in the destructor as everything should have > been done before to not have a parent at this time. That's why I say the fix > should not be there imo but more in _eo_unref. eo_del should just call eo_unref then and put it in one place. but we shouldnt have to put this in multiple places. having eo_unref not del an obj when we hit 0 refs imho is a bad idea. -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ras...@rasterman.com ------------------------------------------------------------------------------ _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
