Re: [Qemu-devel] [PATCH v2 03/27] qom: clean up/optimize object_dynamic_cast

[Anthony Liguori <anthony@codemonkey.ws>]

On 02/04/2012 02:02 AM, Paolo Bonzini wrote:
> The interface loop can be performed only on the parent object.  It
> does not need to be done on each interface.  Similarly, we can
> simplify the code by switching early from the implementation
> object to the parent object.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   qom/object.c |   38 ++++++++++++++++++--------------------
>   1 files changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index 4261944..4d21f0a 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -372,7 +372,6 @@ static bool object_is_type(Object *obj, const char *typename)
>   {
>       TypeImpl *target_type = type_get_by_name(typename);
>       TypeImpl *type = obj->class->type;
> -    GSList *i;
>
>       /* Check if typename is a direct ancestor of type */
>       while (type) {
> @@ -383,15 +382,6 @@ static bool object_is_type(Object *obj, const char *typename)
>           type = type_get_parent(type);
>       }
>
> -    /* Check if obj has an interface of typename */
> -    for (i = obj->interfaces; i; i = i->next) {
> -        Interface *iface = i->data;
> -
> -        if (object_is_type(OBJECT(iface), typename)) {
> -            return true;
> -        }
> -    }
> -
>       return false;
>   }

So this changes object_is_type() to only determine if the the object is a direct 
ancestor.  I think it's worth changing the name to something like 
object_is_ancestor_of() or something like that.

>
> @@ -404,6 +394,24 @@ Object *object_dynamic_cast(Object *obj, const char *typename)
>           return obj;
>       }
>
> +    /* Check if obj is an interface and its containing object is a direct
> +     * ancestor of typename.  In principle we could do this test at the very
> +     * beginning of object_dynamic_cast, avoiding a second call to
> +     * object_is_type.  However, casting between interfaces is relatively
> +     * rare, and object_is_type(obj, TYPE_INTERFACE) would fail almost always.
> +     *
> +     * Perhaps we could add a magic value to the object header for increased
> +     * (run-time) type safety and to speed up tests like this one.  If we ever
> +     * do that we can revisit the order here.
> +     */
> +    if (object_is_type(obj, TYPE_INTERFACE)) {
> +        assert(!obj->interfaces);
> +        obj = INTERFACE(obj)->obj;
> +        if (object_is_type(obj, typename)) {
> +            return obj;
> +        }
> +    }
> +
>       /* Check if obj has an interface of typename */
>       for (i = obj->interfaces; i; i = i->next) {
>           Interface *iface = i->data;
> @@ -413,16 +421,6 @@ Object *object_dynamic_cast(Object *obj, const char *typename)
>           }
>       }
>
> -    /* Check if obj is an interface and its containing object is a direct
> -     * ancestor of typename */
> -    if (object_is_type(obj, TYPE_INTERFACE)) {
> -        Interface *iface = INTERFACE(obj);
> -
> -        if (object_is_type(iface->obj, typename)) {
> -            return iface->obj;
> -        }
> -    }
> -
>       return NULL;
>   }

This looks pretty right to me.  We really need a unit test for this stuff.  I've 
got one in my tree, I'll dig it out so we can start more rigorously validating 
these different casting cases.

Regards,

Anthony Liguori

>