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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-06-10 at 16:49 +0100, Wei Liu wrote:
> 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.

I think it should return None in this case and then you use
      if init_val:
          s += "if (%s != %s) {\n" % (fexpr, init_val)
      else: 
          s += "if (%s) {\n" % (fexpr)

as discussed previously. Or maybe you could make get_init_val return the
complete appropriate expression and call it get_default_expr (or
something better than that)

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

Thanks.

Ian.


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