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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Jun 10, 2014 at 02:25:14PM +0100, Ian Campbell wrote:
> 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?
> 

Because it's an opaque type and I didn't bother to modify the code
generator to have it generate some other functions to return value from
opaque type, as libxl_defbool is the only one that needs extra care so
far.

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

OK, I'm fine with this.

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

Not true for libxl_defbool, as stated above, generating code like
   if (some_defbool_field)
won't work.

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

IIRC with this line Enumeration type doesn't have a default value.

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

Yes, you're right on this one.

> >  
> > -libxl_defbool = Builtin("defbool", passby=PASS_BY_REFERENCE)
> > +libxl_defbool = Builtin("defbool", passby=PASS_BY_REFERENCE,
> > +                        init_val="LIBXL__DEFBOOL_DEFAULT")
> 
> Or this.
> 

I would rather be explicit than implicit. LIBXL__DEFBOOL_DEFAULT happens
to be 0 and that's all. If we ever change our internal implementation
(however unlikely), generator will generate wrong thing. And I bet it
won't be very pleasant to debug 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.
> 

I did that in my previous round but you said you wouldn't worry about
readability of generated code (albeit I also modified those "0"s).

So you think it's only necessary to have non-zero default using LIBXL_*
defines? I'm fine with this.

Wei.

> 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