Re: [E-devel] [EGIT] [core/efl] master 03/03: eo: if EO_DEBUG, check if efl_super() object 'isa' the given class.
On Mon, Dec 5, 2016 at 3:16 PM, Tom Hacohen wrote: > This is wrong, and as JP and Stefan said, breaks things. First of all, > why add code to the non-debug variation? Also, if you're already adding > to the non-debug variation, why not include it in the commit message? > This is very misleading. sorry not splitting the commits as very small pieces, but the idea was to check the object since the class was already being checked... the check _is_a_obj() is as check as the class one, so that's why it's out of the EO_DEBUG guards. > Anyhow, as the tests clearly show, efl_super() is also used with class > functions, so it needs to work. The efl_isa() line should work on > classes too (I think), but the is_object is plain-wrong, and definitely > shouldn't be outside the ifdef, but also not inside. It just doesn't > belong there. as above, I'm rather going to add "|| is_a_class()" like done for class than hide it under EO_DEBUG due those reasons. efl_isa() is more expensive, then okay to remain solely on EO_DEBUG. > Furthermore, while I'm all in favour of this test in the eo_debug > scenario, I disagree with the statement in the commit message. If you > have such mistakes you are doing something very wrong, because the > recommended way of using efl_super is with the macro "MY_CLASS" that > should be defined to the current relevant class in the source file in > order to avoid such mistakes as you described... it happens, to start the first time I saw efl_super() was in code generated by eolian, that uses the name, not MY_CLASS... then I reached the c&p errors I said. If you do check my code in git, you see I converted to MY_CLASS once I discovered about it by reading other code. In all cases I'm far from a "n00b" and faced those errors and when I expected the system to be nicer to me, thus I added these errors checks. Also remember that users may have 2+ objects in the same C file and the misuse is even easier... even with the macro as you may naively reorder the code and break, then a proper message like the one with efl_isa() comes to rescue. If we should just blindly trust efl_super() is going to be used properly, then why check for class? that check is what remembered me of my early mistakes and lead to the extra checks on object. -- Gustavo Sverzut Barbieri -- Mobile: +55 (16) 99354-9890 -- ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [EGIT] [core/efl] master 03/03: eo: if EO_DEBUG, check if efl_super() object 'isa' the given class.
This is wrong, and as JP and Stefan said, breaks things. First of all, why add code to the non-debug variation? Also, if you're already adding to the non-debug variation, why not include it in the commit message? This is very misleading. Anyhow, as the tests clearly show, efl_super() is also used with class functions, so it needs to work. The efl_isa() line should work on classes too (I think), but the is_object is plain-wrong, and definitely shouldn't be outside the ifdef, but also not inside. It just doesn't belong there. Furthermore, while I'm all in favour of this test in the eo_debug scenario, I disagree with the statement in the commit message. If you have such mistakes you are doing something very wrong, because the recommended way of using efl_super is with the macro "MY_CLASS" that should be defined to the current relevant class in the source file in order to avoid such mistakes as you described... -- Tom. On 02/12/16 16:51, Gustavo Sverzut Barbieri wrote: > barbieri pushed a commit to branch master. > > http://git.enlightenment.org/core/efl.git/commit/?id=04450c4ee03fd20271ec4f911667acea10ba1375 > > commit 04450c4ee03fd20271ec4f911667acea10ba1375 > Author: Gustavo Sverzut Barbieri > Date: Fri Dec 2 14:49:06 2016 -0200 > > eo: if EO_DEBUG, check if efl_super() object 'isa' the given class. > > A common error is to copy & paste efl_super() calls and forget to fix > the class. If usin EO_DEBUG, then use efl_isa() to error. > --- > src/lib/eo/eo.c | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/src/lib/eo/eo.c b/src/lib/eo/eo.c > index 9719034..e86f052 100644 > --- a/src/lib/eo/eo.c > +++ b/src/lib/eo/eo.c > @@ -318,6 +318,11 @@ efl_super(const Eo *obj, const Efl_Class *cur_klass) > { > EO_CLASS_POINTER_GOTO(cur_klass, klass, err); > > + if (EINA_UNLIKELY(!_eo_is_a_obj(obj))) goto err_obj; > +#ifdef EO_DEBUG > + if (EINA_UNLIKELY(!efl_isa(obj, cur_klass))) goto err_obj_hierarchy; > +#endif > + > /* FIXME: Switch to atomic operations intead of lock. */ > eina_spinlock_take(&_super_class_lock); > _super_class = klass; > @@ -326,6 +331,14 @@ efl_super(const Eo *obj, const Efl_Class *cur_klass) > err: > _EO_POINTER_ERR("Class (%p) is an invalid ref.", cur_klass); > return NULL; > +err_obj: > + _EO_POINTER_ERR("Object (%p) is an invalid ref, class=%p (%s).", obj, > cur_klass, efl_class_name_get(cur_klass)); > + return NULL; > +#ifdef EO_DEBUG > +err_obj_hierarchy: > + _EO_POINTER_ERR("Object (%p) class=%p (%s) is not an instance of class=%p > (%s).", obj, efl_class_get(obj), efl_class_name_get(obj), cur_klass, > efl_class_name_get(cur_klass)); > +#endif > + return NULL; > } > > EAPI Eina_Bool > -- ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [EGIT] [core/efl] master 03/03: eo: if EO_DEBUG, check if efl_super() object 'isa' the given class.
Hello. On 02/12/16 17:51, Gustavo Sverzut Barbieri wrote: > barbieri pushed a commit to branch master. > > http://git.enlightenment.org/core/efl.git/commit/?id=04450c4ee03fd20271ec4f911667acea10ba1375 > > commit 04450c4ee03fd20271ec4f911667acea10ba1375 > Author: Gustavo Sverzut Barbieri > Date: Fri Dec 2 14:49:06 2016 -0200 > > eo: if EO_DEBUG, check if efl_super() object 'isa' the given class. > > A common error is to copy & paste efl_super() calls and forget to fix > the class. If usin EO_DEBUG, then use efl_isa() to error. > --- Git bisect pointed me to this commit as the beginning from failed tests in the eo test suite. In this case tests/eo/test_function_overrides I have seen other eo tests failing as well but they seemt o come from later changes so it would be a nested git bisect which I'm not motivated to do on a Monday morning. :) On current HEAD I see the following EO tests failing here and on Jenkins: FAIL: tests/eo/test_function_overrides FAIL: tests/eo/eo_suite FAIL: tests/eo/eo_suite_add_fallback Gustavo, please run make check and fix the recent additions to make sure the EO test suite passes. Thanks. regards Stefan Schmidt -- ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [EGIT] [core/efl] master 03/03: eo: if EO_DEBUG, check if efl_super() object 'isa' the given class.
Hey Gustavo, On 3 December 2016 at 01:51, Gustavo Sverzut Barbieri wrote: > barbieri pushed a commit to branch master. > > http://git.enlightenment.org/core/efl.git/commit/?id= > 04450c4ee03fd20271ec4f911667acea10ba1375 > > commit 04450c4ee03fd20271ec4f911667acea10ba1375 > Author: Gustavo Sverzut Barbieri > Date: Fri Dec 2 14:49:06 2016 -0200 > > eo: if EO_DEBUG, check if efl_super() object 'isa' the given class. > > A common error is to copy & paste efl_super() calls and forget to fix > the class. If usin EO_DEBUG, then use efl_isa() to error. > This patch broke make check as there are tests with efl_super on a class. See _class_print() in tests/eo/function_overrides2.c > --- > src/lib/eo/eo.c | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/src/lib/eo/eo.c b/src/lib/eo/eo.c > index 9719034..e86f052 100644 > --- a/src/lib/eo/eo.c > +++ b/src/lib/eo/eo.c > @@ -318,6 +318,11 @@ efl_super(const Eo *obj, const Efl_Class *cur_klass) > { > EO_CLASS_POINTER_GOTO(cur_klass, klass, err); > > + if (EINA_UNLIKELY(!_eo_is_a_obj(obj))) goto err_obj; > This fails as the obj is a Efl_Class. > +#ifdef EO_DEBUG > + if (EINA_UNLIKELY(!efl_isa(obj, cur_klass))) goto err_obj_hierarchy; > +#endif > + > /* FIXME: Switch to atomic operations intead of lock. */ > eina_spinlock_take(&_super_class_lock); > _super_class = klass; > @@ -326,6 +331,14 @@ efl_super(const Eo *obj, const Efl_Class *cur_klass) > err: > _EO_POINTER_ERR("Class (%p) is an invalid ref.", cur_klass); > return NULL; > +err_obj: > + _EO_POINTER_ERR("Object (%p) is an invalid ref, class=%p (%s).", obj, > cur_klass, efl_class_name_get(cur_klass)); > + return NULL; > +#ifdef EO_DEBUG > +err_obj_hierarchy: > + _EO_POINTER_ERR("Object (%p) class=%p (%s) is not an instance of > class=%p (%s).", obj, efl_class_get(obj), efl_class_name_get(obj), > cur_klass, efl_class_name_get(cur_klass)); > +#endif > + return NULL; > } > > EAPI Eina_Bool > > -- > Best regards, -- Jean-Philippe André -- ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel