On Wed, 03 Jan 2018 09:36:34 +0000 Andrew Williams <a...@andywilliams.me> said:
> Wow, I guess I walked into that with my "... whatever ..." so let's go 1
> step simpler still:
>
> void some_api(Eo *parent) {
> Eo *var = efl_add(SOME_CLASS, parent);
>
> printf("I got an Eo %p, %s\n", var, efl_name_get(var));
>
> // TODO figure whether or not I should release var
> efl_unref(var); // ???
> }
>
> This is still not clear as we don't know what parent was used. Compare to:
>
> void some_api(void) {
> char *ptr = malloc(10);
>
> printf("Got pointer %p\n", ptr);
>
> free(ptr);
> }
>
> Here it is clear. malloc always returns a memory handle that must be freed.
>
> Are you seeing the confusion yet? I don't think I have it in me to explain
> any further.
the point i made about using parent var i made before stands. the code screams
to me "i must have a parent". otherwise NULL would be used. the majority
(probably vast majority) of objects will probably need a parent, thus what we
have is optimized to be short, simple and sweet for the majority of use cases.
if we added a "no parent" version which would make the most sense
(efl_noparent_add) since it would make the less common case more wordy, then
we'd need to add variants also for the _ref case (efl_noparent_add_ref) so we'd
be adding 2, not 1. is having "rarely used constructors" added to what people
need to learn and remember a good thing just to avoid passing NULL as a parent?
(because accompanying this change would make NULL parents an invalid param and
need to cause efl_add to fail).
i have doubts if this is a good thing. it raises the learning curve. but then
there is all the work needed to retrofit the code we have to match this core
change. let's just assume the change os 50/50 good/bad ... which i think it's
not even there, but let's say it's even. there is a lot of work needed to
change efl's codebase to match and this (1571 instances from my count right
now). you have to get all those 1500+ changes correct. likely 80% will be able
to be gotten right. some of these need fixing too like test_efl_anim_*.c which
use NULL parents... without a loop to run these tests... how will they sensibly
work? yup. they rely on ecore_animators internally... but this is wrong for
efl's design direction... these should have. in fact must have parent objects
(that trace back to a loop - the main loop in this case).
> What I really want to avoid is having people confused by our new API and
> having to review this again in retrospect a few years down the line.
I get what you are saying. I think that a NULL parent is going to be a rare
case. it's where you have created a floating object (or very rare toplevel tree
objects like loops but these will be created for you like the mainloop one
is, as a part of creating a thread object which WILL be a child of your
current loop object IMHO). and as a floating object... i point to the
discussion surrounding gtk and prior votes/threads etc. :) i disagree that this
is a "the sky is falling" issue. i think it's one of those "it's not perfect
but the alternative is just as bad if not a little worse" situations.
> Andy
>
> On Wed, 3 Jan 2018 at 03:59 Carsten Haitzler <ras...@rasterman.com> wrote:
>
> > On Tue, 02 Jan 2018 19:19:37 +0000 Andrew Williams <a...@andywilliams.me>
> > said:
> >
> > > Hi,
> > >
> > > I am obviously struggling to explain this with words, so will try with
> > code.
> > > Take the following code fragment:
> > >
> > > void some_api(Eo *parent) {
> > > Eo *var = efl_add(SOME_CLASS, parent);
> > >
> > > ... do some stuff with var (possible, acknowledged, race condition)
> > ...
> > >
> > > // TODO figure whether or not I should release var
> > > efl_unref(var); // ???
> > > }
> > >
> > > In that code how can I know the correct behaviour without inspecting the
> > > content of parent?
> >
> > the same as:
> >
> > void some_api(...) {
> > char *ptr = malloc(10);
> > ...
> > free(ptr); // ???
> > }
> >
> > you do NOT know if you are to free or not ... it depends what you do with
> > ptr
> > between malloc and end of scope. do you pass the ptr to another func to
> > consume it or not?stick it in a list or array to store it? this is life in
> > C. C
> > is not java.
> >
> > parent here is an explicit store like a list or array. store this object in
> > this parent (if not null). it's explicit.
> >
> > > If I am a user of this API I would expect to be able to read code and
> > > understand if the correct objects have been released.
> > > With the last line this may be released too soon, without it we may have
> > a
> > > memory leak...
> >
> > reading your sample code says to me "this object REQUIRES a parent"
> > otherwise
> > why would you pass in a parent var at all? you'd use NULL if it doesn't.
> >
> > > Does that help?
> > > Andy
> > >
> > > On Wed, 27 Dec 2017 at 07:15 Carsten Haitzler <ras...@rasterman.com>
> > wrote:
> > >
> > > > On Tue, 26 Dec 2017 17:46:50 -0200 Felipe Magno de Almeida
> > > > <felipe.m.alme...@gmail.com> said:
> > > >
> > > > > 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.
> > > >
> > > > that's actually what happens, why efl_add_ref ends up with a refcount
> > of
> > > > 2, and
> > > > efl_add has a refcount of 1 (if parent is non-null). efl_add if parent
> > is
> > > > not
> > > > null is doing a parent_set during add. it's taking the "convenience"
> > that
> > > > the
> > > > parent is transferred from scope to parent just so you dont have to
> > unref
> > > > at
> > > > end of scope manually - in c that is. we're really just talking c here.
> > > >
> > > > > 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,
> > > >
> > > > that was precisely the vote and discussion back in 2014. so while what
> > we
> > > > have
> > > > in c is not "strictly correct" (so to speak) it's far more convenient
> > than
> > > > forcing people to manually unref at end of scope.
> > > >
> > > > > 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.
> > > >
> > > > that'd be efl_add_ref() for bindings where scope is auto-managed by the
> > > > language (as above), and efl_add for c. if you want to write c in the
> > > > "strict
> > > > scope references" way like these other languages, then efl_add_ref is
> > > > there for
> > > > that. it's going to be inconvenient and requires you to handle scope
> > > > cleaning
> > > > correctly. with some gcc extensions this can actually be automated,
> > but the
> > > > problem is we have to have our api work without such extensions.
> > > >
> > > > you can use __attribute__ ((__cleanup__(efl_unref))) on such vars in
> > > > gcc... but
> > > > it's non-standard. if this was standard across all major compilers then
> > > > this
> > > > whole argument would be moot. we're have efl_add behave like
> > efl_add_ref
> > > > all
> > > > the time and require all objects handles to be unique in scope and have
> > > > some
> > > > macro to declare the object handle with the above attributes and you
> > have
> > > > to
> > > > compile with -fexceptions to handle c++ exception unwinding if it goes
> > > > through
> > > > some c code along the way.... but as i said. it's non-standard. we
> > can't
> > > > rely
> > > > on it.
> > > >
> > > > > 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.
> > > >
> > > > that was my take too. it should be unrefed only once. it smells of a
> > bug
> > > > to me.
> > > > i have very rarely played (i think just once or twice) with
> > efl_add_ref().
> > > > i've
> > > > just use efl_ref() and efl_unref().
> > > >
> > > > > 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
> > > > >
> > > >
> > > >
> > > > --
> > > > ------------- 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
> > > >
> > > --
> > > 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
