JP, Cedric, me and TAsn have been having this argument for a while. IMO, the whole problem is that we're thinking of ownership in terms of parentship, which is wrong with reference counting. Ownership is shared between all reference owners and that's it. That's also the only sane way for bindings to work without having references being pull under their feet.
The whole efl_del argument just exist because it is kinda poorly named. IMO, del means: get this object to an "empty" state. Just like close to files and hide and unparent to UI objects. efl_del should not steal references under people who owns it, the object would get deleted at a later time when everybody using the object stops doing so, we could even return errors from efl_del'eted objects for methods that do not make sense anymore, causing most actions to, possibly, halt earlier rather than later. IMO, the whole problem with efl_add/efl_add_ref is that "parents" are treated specially, which they should not. parent_set should increment efl_ref and parent_unset should decrement it. For C, OTH, where we do expect some "automatism" on resource handling, efl_unref'ing may be too much of a hassle when a parent is already going to handle the lifetime of the object. So, it would make sense, IMO, for efl_add_scope. It could even be that efl_add_scope is named efl_add, no problem, as long as there's a efl_add that keeps this semantics for binding development. Which also means that parent_set/unset must be fixed. Also, @own tags must _not_ relate to parent_set, because that has no useful information for tags or users, if needed we can add a @reparent tag, but that's not really special information such as real owernship information. So, #4 on the original OP should be treated as a bug if we consider efl_add as efl_add_scope, but we also need to fix parent_set/parent_unset. And code must not unref more than they ref. Regards, On Tue, Dec 26, 2017 at 10:21 AM, Andrew Williams <a...@andywilliams.me> wrote: > In the books I have read they consider null being a special case of > behaviour within a method just as confusing as passing a bool like in your > illustration. > > Andy > > On Tue, 26 Dec 2017 at 12:19, Andrew Williams <a...@andywilliams.me> wrote: > >> Hi, >> >> With the proposal of efl_add and efl_add_child we remove the need for >> efl_add_ref* as the result of the former becomes consistent in its return >> of owned or not owned references. >> Hopefully Cedric can confirm this as I don’t know the spec. >> Right now we have a second one because the first is not consistently >> returning pointers that either do or do not need to be unrefd. >> >> Andy >> On Tue, 26 Dec 2017 at 04:25, Carsten Haitzler <ras...@rasterman.com> >> wrote: >> >>> On Sun, 24 Dec 2017 09:16:08 +0000 Andrew Williams <a...@andywilliams.me> >>> said: >>> >>> > Are you trolling me now? >>> >>> no. you said its inconsistent. it's consistent. it has a simple rule. >>> >>> http://www.dictionary.com/browse/consistent >>> >>> it consistently adheres to the same principles. i described them. >>> >>> > A method that does two different things depending on some magic value / >>> >>> it's not a MAGIC value. it's a parent object handle. it's far from magic. >>> it's >>> "put this object into THIS box here, or into NO box and give it to me" >>> based on >>> that option. >>> >>> > null flag is a code smell (see Clean Coders if this is not familiar). >>> > Consider this method: >>> > >>> > ptr get_mem(string poolname, long bytes) { >>> > If (poolname == NULL) >>> > return malloc(bytes); // MUST be freed >>> > else >>> > return get_pool(poolname).borrow(bytes); // must NOT be freed >>> > } >>> > >>> > Do you think that is consistent? The user is not sure without inspecting >>> > the parameter contents whether or not the should free(). This is >>> > conceptually what we are setting up. >>> >>> #define efl_add_noparent(klass, ...) efl_add(klass, NULL, ## __VA_ARGS__) >>> >>> happy? you can have a macro to hide the parent if NULL. but it'll be used >>> fairly rarely. >>> >>> > Back to our efl_add - what would be consistent is this: >>> > >>> > Eo* efl_add(klass, ... constructors ...); // must be unrefd (no parent) >>> > >>> > Eo* efl_add_child(klass, parent, ... constructors ... ); // parent must >>> not >>> > be null, should not be unrefd >>> > >>> > That is consistent. It is also compliant with the V7 vote. It still has >>> the >>> > race condition but is much easier to read. I know from the method names >>> > what is expected. >>> >>> your proposal was to have efl_add return void. the above is better for >>> sure. i >>> see we were walking down similar paths. >>> >>> i still dislike the above because it just makes the api more verbose for >>> the >>> sake of special-casing "parent == NULL". i dislike it. this isn't a magic >>> "bool" that turns on or off behaviour. that is actually not great. you >>> read code >>> like >>> >>> func_do_something(obj, EINA_TRUE, EINA_FALSE, EINA_TRUE); >>> >>> ... and what are the 3 bools? it's not clear or obvious. but with a >>> constructor >>> like efl_add firstly it's incredibly common so it's something that will be >>> learned very quickly, and the typing already tells you it's an object >>> handle. >>> and you should have learned already that it's the parent object (or no >>> parent >>> at all if NULL). >>> >>> why do i dislike it? we now go from 2 constructors (efl_add and >>> efl_add_ref) to >>> 4 (efl_add, efl_child_add, efl_add_ref, efl_child_add_ref). i dislike this >>> "explosion" just to hide the parent arg being NULL. >>> >>> > Thoughts? >>> > On Sun, 24 Dec 2017 at 03:33, Carsten Haitzler <ras...@rasterman.com> >>> wrote: >>> > >>> > > On Sat, 23 Dec 2017 11:30:58 +0000 Andrew Williams < >>> a...@andywilliams.me> >>> > > said: >>> > > >>> > > > Hi, >>> > > > >>> > > > As this thread seems to be descending into word games and (insert >>> > > > appropriate word) contests I will reiterate my concern: >>> > > > >>> > > > efl_add is inconsistent and that should be addressed. >>> > > >>> > > do it's not. i explained already that it is not. i'll repeat again. >>> it's >>> > > consistent: >>> > > >>> > > if parent == valid object, then ref is owned by parent >>> > > else ref is owned by caller/scope. >>> > > >>> > > that is consistent. >>> > > >>> > > > I hope that is clear enough >>> > > > Andy >>> > > > >>> > > > >>> > > > On Thu, 21 Dec 2017 at 13:15, Andrew Williams <a...@andywilliams.me >>> > >>> > > wrote: >>> > > > >>> > > > > 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 >>> > > > > >>> > > > > 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. >>> > > > > >>> > > > > 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. >>> > > > > >>> > > > > 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 >>> > > > > >>> > > > -- >>> > > > 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 >> > -- > 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 -- Felipe Magno de Almeida ------------------------------------------------------------------------------ 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
