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

[Wei Liu <wei.liu2@citrix.com>]

On Tue, Jun 10, 2014 at 03:56:10PM +0100, Ian Campbell wrote:
[...]
> > > > -        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.
> 

No problem.

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

Good idea.

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

Yes, it's None if it's not explicitly set.

If I make get_init_val function return 0 when it encounters None then I
don't need this hunk anymore.

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

Ack.

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

This is rather trivial after all. I can fix this in my next version.

Wei.