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

[Ian Campbell <Ian.Campbell@citrix.com>]

On Tue, 2014-06-10 at 14:43 +0100, Wei Liu wrote:
> 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@citrix.com>
> > > ---
> > >  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.

Ah, ok. BTW I think "ty != libxl_debool" is valid. Or maybe
isinstance(), whichever one works is better than a string compare I
think.

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

Oh yes. Ick.

Can you perhaps abstract this into:

def get_field_val(ftype, fexpr):
    ftype == libxl_defbool:
       return "%s.val" % fexpr
    else:
       return fexpr
    
?
> 
> > 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.
           ^out (per your other mail)

It should be None, which I think is set by the following Type.__init__.

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

The assumption that the default bit pattern for any type is all zeroes
is pretty firmly baked into lots of places. It is incredibly unlikely
that this is going to change.

> > 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 I did, I started off mentioning the unlogged
s/1/LIBXL_VGA_INTERFACE_TYPE_CIRRUS/ but then didn't notice that we got
on to talking about 0s too. Sorry.

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

If they have an init_val at all I think it should use the names not the
numbers. If the default is 0 then it needn't be specified explicitly
(and in that case it'll be if (x) not if (x==THING) in the generated
code)

But no need to fix this unless you want to, it's just a minor wrinkle in
the existing thing.

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