On Fri, 22 Dec 2017 10:10:48 +0000 Andrew Williams <a...@andywilliams.me> said:
> On Fri, 22 Dec 2017 at 09:59 Carsten Haitzler <ras...@rasterman.com> wrote: > > > On Fri, 22 Dec 2017 09:43:20 +0000 Andrew Williams <a...@andywilliams.me> > > said: > > > > > Hi, > > > > > > I think that maybe I could have explained this a little better, let me > > step > > > back from the implementation details for a minute. > > > > > > As a user of this API all I want to know is do I have to unref a > > reference > > > that I have been handed. From this point of view we should be consistent. > > > My proposal was (intended) to mean that efl_add should never require the > > > user to unreference whereas efl_add_ref should always require this (but > > > only once - that may have been a bug). > > > > > > Having to know the value of the second parameter to the method to > > ascertain > > > the correct behaviour is what I think is confusing and should be > > > eliminated. Therefore the main purpose was so that the docs could ready > > > simply "this is a borrowed reference which you should not unref" vs "this > > > is an owned reference and you must unref when done". > > > > yes. i got that. but your proposal is no better than what is there. in fact > > think it's worse. unless i didn't understand it? > > > > "is no better"? it ensures that you know to unref efl_add_ref and that you > cannot accidentally use a risky efl_add. Not ideal which is why I suggested > efl_add_scope in a follow up which is something you don't have to unref but > also guarantees liveness for the scope. forcing all this code that builds widgets (see below about leaks) to remember to unref is asking for disaster as people will juyst end up using efl_Add_ref all the time because not doing so it incredibly inconvenient. it's also going to be a major complaint in usage. reality is ... people seem fine with how we do it in c. why? this is how gtk works... same thing. it's also how efl has worked for a long time. hello world for gtk: https://developer.gnome.org/gtk-tutorial/2.90/c39.html https://developer.gnome.org/gtk3/stable/gtk-getting-started.html the construction return an object ptr, you do things with it, hand it to a parent. no unreffing. in gtk the parent could delete the children any time it liked like in efl. this is a common design pattern already in c for this kind of stuff. you can be pedantic, or you can just be practical. this is one of these "we're being practical" things rather than pedantic. if you want to write code pedantically then efl_add_ref() is there - remember to unref when going out of scope or you'll have leaks. > > i get your point... as i said. but your proposal doesn't sound as if it's > > better. > > > > what MIGHT be better is to class objects into "can have a null parent" and > > "must have a non-null parent". the 1st case doesn't change. from now. the > > 2nd > > case results in efl_add returning NULL (construction fails) if the parent > > is > > null. that might be an improvement. > > > > Why do I, as the user, have to care what the content of the parent variable > is? It was suggested to me that efl_add could require a parent and then > would safely return the reference, but you still have the race condition so > I'm not convinced. see above. gtk and friends are full of this condition yet people are fine with it. people have been fine with this in efl for a long time. it's not a race condition really. it's a lifecycle behavior definition. once you hand a child to a parent, that parent can and will control lifecycle and thus it MAY delete it at some point... and that point MAY be immediately in some rare cases. we have a single efl_add() constructor do a multi-step "create, set properties/callbcks and set parent etc. etc.". so yes - knowing what parent is matters because of this. > > > The added complication is the race condition with the "parent" usage in > > > efl_add. When I pass a parent reference to efl_add then the returned > > handle > > > is something I can use. Yes this is very helpful. But this is dangerous. > > We > > > (as a framework) are providing 0 guarantee that the object will live as > > > long as the scope - that is entirely up to the parent, not the user and > > not > > > the framework. This is also something that I think we should address. > > > > we can't guarantee it if the parent decides some time within the scope to > > delete the object you added. well we can;'t unless you use efl_add_ref() > > then > > you create an object with 2 references (parent one and your scope/app one) > > and > > you will have to remember to unref at end of scope. this leads to verbose > > code, > > and having to remember to do this... which is more than likely going to > > lead to > > huge amounts of memory leaking. > > Only if not coded correctly. We currently return references which are not > clear if they should be unref or not so there is not a huge difference. it is clear. if parent is NULL - YOU own (as the caller) the reference. unref/del as appropriate. if parent is NOT NULL then parent owns the ref and the object lifecycle will be managed by the parent. what is not clear about this? how is "well if you write it correctly and remember to add all the unref's when exiting scope" better? it's MORE code and MORE places for things to go wrong. it's also not a common pattern in C. see the gtk samples and legacy efl api too. > > so which is worse? sometimes a parent decides to delete an object on you > > while > > you are still holding and using its handle, or that you always have to > > unref > > every obj you added when you exit scope? i'll take the first one any day > > of the > > week. :) > > > > I'm going to say safety. 100% safety is more important than removing a line > of code. This is our opportunity to do things right. Introducing a race > condition at the root of our API seems an unfortunate choice. then you are free to use efl_add_ref(). reality is that 99.9% of the time objects will not be deleted within scope where they are created and set up because having this kind of behaviour is kind of nasty and it should be avoided. we have safety thanks to eoid anyway. so you won't get a crash.. you'll get an error print and things will recover just fine. because there is no automatic unreffing when exiting scope in C, requiring people to do this by hand is going to lead a world of leaks where people continue to forget doing this as this is not a rare thing but a common thing to do, and it's going to lead to more verbose code... in return for what? safety isn't improved. eoid took care of that already. this whole discussion was already had like 2+ years ago when the core design for eo was being hashed out. tom was like you wanting something pedantically correct requiring unrefs at end of scope etc. yes i know. the option of an add that doesn't return anything. that's kind of a very half-baked and not useful api. he ended up agreeing that what we have now is not perfect, but it's the best option for sanity. now we're having this discussion again and TBH i think it's a bit late. there is a mountain of code that now depends on this design. even if we were to choose to change - it'd be a nightmare to do and have efl in a horribly unstable state for weeks if not months. ever use where the return is used has to become an efl_add_ref, and those that done become efl_add so every instance needs inspection etc. and someone has to put the efl_unrefs in all of the scope exit points AND get it all right... this is the kind of change that would only really justify being done if we had a showstopper design bug. we do not. not IMHO. there is clear behaviour and it's logical and simple. if you don't LIKE it... efl_add_ref is there for you to use and then to remember to unref on all scope exits by hand. i certainly would not choose to use this. > > > So there you go - the background behind my proposal. Hopefully if I did > > not > > > explain the details well you can now see why I felt it is important. > > > > yup. i know the background. i'm speaking of the details of the proposal. > > > > > Thanks, > > > Andy > > > > > > On Fri, 22 Dec 2017 at 03:25 Carsten Haitzler <ras...@rasterman.com> > > wrote: > > > > > > > On Thu, 21 Dec 2017 13:15:26 +0000 Andrew Williams < > > a...@andywilliams.me> > > > > said: > > > > > > > > > Hi, > > > > > > > > > > This is now well documented ( > > > > > https://www.enlightenment.org/develop/tutorials/c/eo-refcount.md) > > but > > > > the > > > > > more I use efl_add the more I feel it is confusing especially to new > > > > > developers. > > > > > > > > > > In the current model (if I understand it correctly) > > > > > 1) child = efl_add(klass, parent) means the child must NOT be > > > > unfeferenced > > > > > 2) child = efl_add(klass, NULL) means the child should be > > unreferenced > > > > > 3) child = efl_add_ref(klass, parent) means the child must be > > > > unreferenced > > > > > 4) child = efl_add_ref(klass, NULL) somehow means that the child > > should > > > > be > > > > > unreferenced twice > > > > > > > > #4 smells like a bug... 1 is intended. > > > > > > > > > In my opinion 1) and 4) are peculiar and so I provide a proposal to > > fix > > > > > this: > > > > > > > > > > We could change efl_add to return void. It never retains a > > reference. If > > > > > the parent is NULL then it should be automatically unref before > > > > returning. > > > > > Then efl_add_ref would be changed to mirror this and always retain > > > > exactly > > > > > 1 reference - so if parent is NULL there is no double count returned. > > > > > > > > umm... then you are saying efl_add_ref() is exactly the same as > > efl_add() > > > > today. what does this help? and the shorter efl_add now returns > > nothing so > > > > i > > > > can't use the return to usefully access the things i created later on > > like > > > > add > > > > callbacks to it, change the label of a button, delete a window, or use > > it > > > > as a > > > > parent for further adds which is like THE most common use case around > > when > > > > building a ui for example (create box, then create a button and pack > > button > > > > into box. i need the box to be able to do that). unless i use > > efl_add_ref > > > > which > > > > si the same thing as efl_add now. > > > > > > > > > Using this model if an Eo * is returned then I know I have a > > reference > > > > and > > > > > must later unref. > > > > > Any need for using the pointer in an efl_add (which is no longer > > > > returned) > > > > > would still be supported through efl_added within the constructor. > > > > > > > > if efl_add_ref returns an obj with only 1 reference, then this is wrong > > > > above. > > > > if parent is NULL, yes you'd have to unref/del. if parent is not null > > then > > > > there is still only 1 ref and it belongs to the parent object. so > > > > > > > > > What do people think about this? I put the suggestion forward to > > improve > > > > > the symettry with add and unref which is currently confusing. If my > > > > > assumptions above are incorrect please let me know! > > > > > > > > > > Thanks, > > > > > Andy > > > > > -- > > > > > http://andywilliams.me > > > > > http://ajwillia.ms > > > > > > > > > > > ------------------------------------------------------------------------------ > > > > > Check out the vibrant tech community on one of the world's most > > > > > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > > > > > _______________________________________________ > > > > > enlightenment-devel mailing list > > > > > enlightenment-devel@lists.sourceforge.net > > > > > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > > > > > > > > > > > > > > > > > -- > > > > ------------- Codito, ergo sum - "I code, therefore I am" > > -------------- > > > > Carsten Haitzler - ras...@rasterman.com > > > > > > > > -- > > > http://andywilliams.me > > > http://ajwillia.ms > > > > > > -- > > ------------- Codito, ergo sum - "I code, therefore I am" -------------- > > Carsten Haitzler - ras...@rasterman.com > > > > -- > http://andywilliams.me > http://ajwillia.ms -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- Carsten Haitzler - ras...@rasterman.com ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
