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