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