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 14:25:14 +0100 Message-ID: <1402406714.1250.121.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1402317809-26833-12-git-send-email-wei.liu2@citrix.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 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 > --- > 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? > 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) 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. 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. > 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. > > -libxl_defbool = Builtin("defbool", passby=PASS_BY_REFERENCE) > +libxl_defbool = Builtin("defbool", passby=PASS_BY_REFERENCE, > + init_val="LIBXL__DEFBOOL_DEFAULT") Or 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. See libxl_timer_mode for how to do it properly. (Maybe idl.py should do the translation, but lets not go there...)