Re: [Xen-devel] [PATCH v6 11/18] libxl/gentypes.py: don't generate default values

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-06-09 at 13:43 +0100, Wei Liu wrote:
> If a type has init_val defined and a field of the type has the value of
> init_val, there's no need to generate output for that field in JSON
> output. When the parser consumes that generated JSON object, all default
> values should be automatically filled in.
> 
> Also define init_val for enumeration type, string type and libxl_defbool
> type.
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
>  tools/libxl/gentypes.py     |   27 ++++++++++++++++++++++++---
>  tools/libxl/idl.py          |    3 ++-
>  tools/libxl/libxl_types.idl |    3 ++-
>  3 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
> index 01416e7..611c8af 100644
> --- a/tools/libxl/gentypes.py
> +++ b/tools/libxl/gentypes.py
> @@ -135,7 +135,7 @@ def _libxl_C_type_init(ty, v, indent = "    ", parent = 
> None, subinit=False):
>              else:
>                  s += _libxl_C_type_init(f.type, fexpr, "", nparent)
>      else:
> -        if ty.init_val is not None:
> +        if ty.init_val is not None and ty.typename != "libxl_defbool":

Why is libxl_defbool special here?

>              s += "%s = %s;\n" % (ty.pass_arg(v, parent is None), ty.init_val)
>          elif ty.init_fn is not None:
>              s += "%s(%s);\n" % (ty.init_fn, ty.pass_arg(v, parent is None))
> @@ -206,6 +206,13 @@ def libxl_C_type_gen_map_key(f, parent, indent = ""):
>          s = indent + s
>      return s.replace("\n", "\n%s" % indent).rstrip(indent)
>  
> +def get_init_val(f):
> +    if f.init_val is not None:
> +        return f.init_val
> +    elif f.type.init_val is not None:
> +        return f.type.init_val
> +    return None
> +
>  def libxl_C_type_gen_json(ty, v, indent = "    ", parent = None):
>      s = ""
>      if parent is None:
> @@ -255,8 +262,22 @@ def libxl_C_type_gen_json(ty, v, indent = "    ", parent 
> = None):
>          s += "    goto out;\n"
>          for f in [f for f in ty.fields if not f.const and not 
> f.type.private]:
>              (nparent,fexpr) = ty.member(v, f, parent is None)
> -            s += libxl_C_type_gen_map_key(f, nparent)
> -            s += libxl_C_type_gen_json(f.type, fexpr, "", nparent)
> +            init_val = get_init_val(f)
> +            if init_val:
> +                if f.type.typename != "libxl_defbool":
> +                    s += "if (%s != %s) {\n" % (fexpr, init_val)
> +                else:
> +                    s += "if (%s.val != %s) {\n" % (fexpr, init_val)
> +                indent1 = "    "
> +            else:
> +                indent1 = ""

I don't think this is right. If a type doesn't has an init_val then its
init_val is 0 and you should compare as a boolean. e.g.
     if init_val:
        s += "if (%s != %s) {\n" % (fexpr, init_val)
     else: 
        s += "if (%s) {\n" % (fexpr)

This gets rid of the special casing of libxl_defbool (where 0 ==
default) as well as removing a bunch of unnecessary p->foo = 0 from the
generated init fns too.

You could also possibly implement this by having get_init_val return an
explicit "0" as a fallback but I think that will resent in less clear
generated code.

> +            s += libxl_C_type_gen_map_key(f, nparent, indent1)
> +            s += libxl_C_type_gen_json(f.type, fexpr, indent1, nparent)
> +
> +            if init_val:
> +                s += "}\n"
> +
>          s += "s = yajl_gen_map_close(hand);\n"
>          s += "if (s != yajl_gen_status_ok)\n"
>          s += "    goto out;\n"
> diff --git a/tools/libxl/idl.py b/tools/libxl/idl.py
> index 8b118dd..14ca165 100644
> --- a/tools/libxl/idl.py
> +++ b/tools/libxl/idl.py
> @@ -142,6 +142,7 @@ class EnumerationValue(object):
>  class Enumeration(Type):
>      def __init__(self, typename, values, **kwargs):
>          kwargs.setdefault('dispose_fn', None)
> +        kwargs.setdefault('init_val', '0')

I'm not sure about this, but I think you don't need it for the reasons
above.

>          Type.__init__(self, typename, **kwargs)
>  
>          self.value_namespace = kwargs.setdefault('value_namespace',
> @@ -270,7 +271,7 @@ uint64 = UInt(64, json_gen_fn = "libxl__uint64_gen_json")
>  
>  string = Builtin("char *", namespace = None, dispose_fn = "free",
>                   json_gen_fn = "libxl__string_gen_json",
> -                 autogenerate_json = False)
> +                 autogenerate_json = False, init_val = "NULL")

If you do as I suggest above then you don't need this I think.
>  
> -libxl_defbool = Builtin("defbool", passby=PASS_BY_REFERENCE)
> +libxl_defbool = Builtin("defbool", passby=PASS_BY_REFERENCE,
> +                        init_val="LIBXL__DEFBOOL_DEFAULT")

Or this.

BTW, I happened to notice that some enums in the IDL define their
(non-zero) default using the integer form, which isn't wrong but did
leap out at me from the generated code. If you fancy fixing that while
you are in the area that would be great. I noticed this on
libxl_action_on_shutdown and libxl_vga_interface_type.

See libxl_timer_mode for how to do it properly. (Maybe idl.py should do
the translation, but lets not go there...)


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel