Re: [E-devel] [EGIT] [core/efl] master 03/03: eo: if EO_DEBUG, check if efl_super() object 'isa' the given class.

2016-12-05 Thread Gustavo Sverzut Barbieri 
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 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.

2016-12-05 Thread Tom Hacohen 
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.

2016-12-05 Thread Stefan Schmidt 
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.

2016-12-05 Thread Jean-Philippe André 
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