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. > 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. > > 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. > 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. > > 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 ------------------------------------------------------------------------------ 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
