From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v6 11/18] libxl/gentypes.py: don't generate default values Date: Tue, 10 Jun 2014 16:54:26 +0100 Message-ID: <1402415666.9559.2.camel@kazak.uk.xensource.com> References: <1402317809-26833-1-git-send-email-wei.liu2@citrix.com> <1402317809-26833-12-git-send-email-wei.liu2@citrix.com> <1402406714.1250.121.camel@kazak.uk.xensource.com> <20140610134300.GJ11959@zion.uk.xensource.com> <1402412170.20641.19.camel@kazak.uk.xensource.com> <20140610154928.GP11959@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140610154928.GP11959@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: ian.jackson@eu.citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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.