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