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