| Message ID | 1397879804-28711-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 18/04/2014 23:56, Amos Kong ha scritto: > Currently we always add a space after c_type in mcgen(), there is > some redundant space in generated code. The space isn't needed for > points by the coding style. > > char * value; > ^ > qapi_free_NameInfo(NameInfo * obj) > ^ > It's fussy to add checking in each mcgen(), this patch just addes > the necessary space in c_type(), and remove original space in mcgen(). It also makes the generator harder to read to though, and the improvement in the generated code is minor enough that I think the change is overall negative. But I won't oppose the patch if the maintainers are fine with it. Paolo > Signed-off-by: Amos Kong <akong@redhat.com> > --- > scripts/qapi-types.py | 10 +++++----- > scripts/qapi-visit.py | 20 ++++++++++---------- > scripts/qapi.py | 14 +++++++------- > 3 files changed, 22 insertions(+), 22 deletions(-) > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index 10864ef..54a9b89 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -23,7 +23,7 @@ def generate_fwd_struct(name, members, builtin_type=False): > typedef struct %(name)sList > { > union { > - %(type)s value; > + %(type)svalue; > uint64_t padding; > }; > struct %(name)sList *next; > @@ -75,7 +75,7 @@ def generate_struct_fields(members): > pop_indent() > else: > ret += mcgen(''' > - %(c_type)s %(c_name)s; > + %(c_type)s%(c_name)s; > ''', > c_type=c_type(argentry), c_name=c_var(argname)) > > @@ -219,7 +219,7 @@ struct %(name)s > > for key in typeinfo: > ret += mcgen(''' > - %(c_type)s %(c_name)s; > + %(c_type)s%(c_name)s; > ''', > c_type=c_type(typeinfo[key]), > c_name=c_fun(key)) > @@ -251,7 +251,7 @@ extern const int %(name)s_qtypes[]; > > def generate_type_cleanup_decl(name): > ret = mcgen(''' > -void qapi_free_%(type)s(%(c_type)s obj); > +void qapi_free_%(type)s(%(c_type)sobj); > ''', > c_type=c_type(name),type=name) > return ret > @@ -259,7 +259,7 @@ void qapi_free_%(type)s(%(c_type)s obj); > def generate_type_cleanup(name): > ret = mcgen(''' > > -void qapi_free_%(type)s(%(c_type)s obj) > +void qapi_free_%(type)s(%(c_type)sobj) > { > QapiDeallocVisitor *md; > Visitor *v; > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 45ce3a9..8ffed3d 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -38,7 +38,7 @@ def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, base = > > ret += mcgen(''' > > -static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error **errp) > +static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s **obj, Error **errp) > { > Error *err = NULL; > ''', > @@ -149,7 +149,7 @@ def generate_visit_struct(expr): > > ret += mcgen(''' > > -void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp) > +void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp) > { > ''', > name=name) > @@ -166,7 +166,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** > def generate_visit_list(name, members): > return mcgen(''' > > -void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp) > +void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp) > { > GenericList *i, **prev = (GenericList **)obj; > Error *err = NULL; > @@ -193,7 +193,7 @@ void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, > def generate_visit_enum(name, members): > return mcgen(''' > > -void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **errp) > +void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **errp) > { > visit_type_enum(m, (int *)obj, %(name)s_lookup, "%(name)s", name, errp); > } > @@ -203,7 +203,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **e > def generate_visit_anon_union(name, members): > ret = mcgen(''' > > -void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp) > +void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp) > { > Error *err = NULL; > > @@ -279,7 +279,7 @@ def generate_visit_union(expr): > > ret += mcgen(''' > > -void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp) > +void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp) > { > Error *err = NULL; > > @@ -367,13 +367,13 @@ def generate_declaration(name, members, genlist=True, builtin_type=False): > if not builtin_type: > ret += mcgen(''' > > -void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp); > +void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp); > ''', > name=name) > > if genlist: > ret += mcgen(''' > -void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp); > +void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); > ''', > name=name) > > @@ -383,7 +383,7 @@ def generate_enum_declaration(name, members, genlist=True): > ret = "" > if genlist: > ret += mcgen(''' > -void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp); > +void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); > ''', > name=name) > > @@ -392,7 +392,7 @@ void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, > def generate_decl_enum(name, members, genlist=True): > return mcgen(''' > > -void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **errp); > +void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **errp); > ''', > name=name) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index b474c39..421f94f 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -425,23 +425,23 @@ def c_type(name): > if name == 'str': > return 'char *' > elif name == 'int': > - return 'int64_t' > + return 'int64_t ' > elif (name == 'int8' or name == 'int16' or name == 'int32' or > name == 'int64' or name == 'uint8' or name == 'uint16' or > name == 'uint32' or name == 'uint64'): > - return name + '_t' > + return name + '_t ' > elif name == 'size': > - return 'uint64_t' > + return 'uint64_t ' > elif name == 'bool': > - return 'bool' > + return 'bool ' > elif name == 'number': > - return 'double' > + return 'double ' > elif type(name) == list: > return '%s *' % c_list_type(name[0]) > elif is_enum(name): > - return name > + return name + ' ' > elif name == None or len(name) == 0: > - return 'void' > + return 'void ' > elif name == name.upper(): > return '%sEvent *' % camel_case(name) > else: >
On 04/18/2014 09:56 PM, Amos Kong wrote: > Currently we always add a space after c_type in mcgen(), there is > some redundant space in generated code. The space isn't needed for > points by the coding style. Second sentence is awkward; maybe: Avoiding the space when appropriate makes us match the coding style. > > char * value; > ^ > qapi_free_NameInfo(NameInfo * obj) > ^ > It's fussy to add checking in each mcgen(), this patch just addes s/addes/adds/ > the necessary space in c_type(), and remove original space in mcgen(). s/remove/removes the/ > > Signed-off-by: Amos Kong <akong@redhat.com> > --- > scripts/qapi-types.py | 10 +++++----- > scripts/qapi-visit.py | 20 ++++++++++---------- > scripts/qapi.py | 14 +++++++------- > 3 files changed, 22 insertions(+), 22 deletions(-) > union { > - %(type)s value; > + %(type)svalue; Code itself does what it claims. It's a bit harder to read the generator without the space, and I might have added a comment to c_type() explaining that the output string includes the space except for pointer types. I'll let others decide whether to take the patch, but I'm comfortable if the commit message is fixed and you want to add: Reviewed-by: Eric Blake <eblake@redhat.com>
On Sat, Apr 19, 2014 at 03:16:52PM -0600, Eric Blake wrote: > On 04/18/2014 09:56 PM, Amos Kong wrote: > > Currently we always add a space after c_type in mcgen(), there is > > some redundant space in generated code. The space isn't needed for > > points by the coding style. > > Second sentence is awkward; maybe: > > Avoiding the space when appropriate makes us match the coding style. > > > > > char * value; > > ^ > > qapi_free_NameInfo(NameInfo * obj) > > ^ > > It's fussy to add checking in each mcgen(), this patch just addes > > s/addes/adds/ > > > the necessary space in c_type(), and remove original space in mcgen(). > > s/remove/removes the/ > > > > > Signed-off-by: Amos Kong <akong@redhat.com> > > --- > > scripts/qapi-types.py | 10 +++++----- > > scripts/qapi-visit.py | 20 ++++++++++---------- > > scripts/qapi.py | 14 +++++++------- > > 3 files changed, 22 insertions(+), 22 deletions(-) > > > union { > > - %(type)s value; > > + %(type)svalue; > > Code itself does what it claims. It's a bit harder to read the > generator without the space, and I might have added a comment to > c_type() explaining that the output string includes the space except for > pointer types. I'll let others decide whether to take the patch, but > I'm comfortable if the commit message is fixed and you want to add: > > Reviewed-by: Eric Blake <eblake@redhat.com> I queued this patch for long time, I'm also not sure if it's necessary. I planed to send it out when I saw your recent comment about this issue. Michael, what's your opinion? > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 18/04/2014 23:56, Amos Kong ha scritto: >> Currently we always add a space after c_type in mcgen(), there is >> some redundant space in generated code. The space isn't needed for >> points by the coding style. >> >> char * value; >> ^ >> qapi_free_NameInfo(NameInfo * obj) >> ^ >> It's fussy to add checking in each mcgen(), this patch just addes >> the necessary space in c_type(), and remove original space in mcgen(). > > It also makes the generator harder to read to though, and the > improvement in the generated code is minor enough that I think the > change is overall negative. But I won't oppose the patch if the > maintainers are fine with it. The readability hit could be avoided: instead of moving the space from mcgen()'s argument to c_type()'s result, add a "hungry character" to c_type()'s result, then make it eat space to the right in mcgen().
On Tue, Apr 22, 2014 at 09:20:41AM +0200, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > Il 18/04/2014 23:56, Amos Kong ha scritto: > >> Currently we always add a space after c_type in mcgen(), there is > >> some redundant space in generated code. The space isn't needed for > >> points by the coding style. > >> > >> char * value; > >> ^ > >> qapi_free_NameInfo(NameInfo * obj) > >> ^ > >> It's fussy to add checking in each mcgen(), this patch just addes > >> the necessary space in c_type(), and remove original space in mcgen(). > > > > It also makes the generator harder to read to though, and the > > improvement in the generated code is minor enough that I think the > > change is overall negative. But I won't oppose the patch if the > > maintainers are fine with it. Hi Markus, > The readability hit could be avoided: instead of moving the space from > mcgen()'s argument to c_type()'s result, add a "hungry character" to > c_type()'s result, then make it eat space to the right in mcgen(). '\b' is used to eat char in left, but what's the "hungry character" to each char in right? When I added a '\b' to c_type()'s return, there is an arror: error: stray '\10' in program Another solution: I tried to use '\b' to eat the redundant space by following code: | def eat_space(type): | if type[-1] == '*': | return '\b' | return '' | return mcgen(''' | | typedef struct %(name)sList | { | union { | %(type)s %(space)svalue; | uint64_t padding; | }; | struct %(name)sList *next; | } %(name)sList; | ''', | type=c_type(name), | space=eat_space(name), | name=name) I looks still not good ?
Amos Kong <akong@redhat.com> writes: > On Tue, Apr 22, 2014 at 09:20:41AM +0200, Markus Armbruster wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >> > Il 18/04/2014 23:56, Amos Kong ha scritto: >> >> Currently we always add a space after c_type in mcgen(), there is >> >> some redundant space in generated code. The space isn't needed for >> >> points by the coding style. >> >> >> >> char * value; >> >> ^ >> >> qapi_free_NameInfo(NameInfo * obj) >> >> ^ >> >> It's fussy to add checking in each mcgen(), this patch just addes >> >> the necessary space in c_type(), and remove original space in mcgen(). >> > >> > It also makes the generator harder to read to though, and the >> > improvement in the generated code is minor enough that I think the >> > change is overall negative. But I won't oppose the patch if the >> > maintainers are fine with it. > > Hi Markus, > >> The readability hit could be avoided: instead of moving the space from >> mcgen()'s argument to c_type()'s result, add a "hungry character" to >> c_type()'s result, then make it eat space to the right in mcgen(). > > '\b' is used to eat char in left, but what's the "hungry character" to > each char in right? > > When I added a '\b' to c_type()'s return, there is an arror: > error: stray '\10' in program I'm afraid you misunderstood me. Pick a character that cannot occur in valid C code. Make c_type()'s return value end in that character in the cases where you don't want space. Then make mcgen() strip it from its return value along with immediately following space. Questions? [...]
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 10864ef..54a9b89 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -23,7 +23,7 @@ def generate_fwd_struct(name, members, builtin_type=False): typedef struct %(name)sList { union { - %(type)s value; + %(type)svalue; uint64_t padding; }; struct %(name)sList *next; @@ -75,7 +75,7 @@ def generate_struct_fields(members): pop_indent() else: ret += mcgen(''' - %(c_type)s %(c_name)s; + %(c_type)s%(c_name)s; ''', c_type=c_type(argentry), c_name=c_var(argname)) @@ -219,7 +219,7 @@ struct %(name)s for key in typeinfo: ret += mcgen(''' - %(c_type)s %(c_name)s; + %(c_type)s%(c_name)s; ''', c_type=c_type(typeinfo[key]), c_name=c_fun(key)) @@ -251,7 +251,7 @@ extern const int %(name)s_qtypes[]; def generate_type_cleanup_decl(name): ret = mcgen(''' -void qapi_free_%(type)s(%(c_type)s obj); +void qapi_free_%(type)s(%(c_type)sobj); ''', c_type=c_type(name),type=name) return ret @@ -259,7 +259,7 @@ void qapi_free_%(type)s(%(c_type)s obj); def generate_type_cleanup(name): ret = mcgen(''' -void qapi_free_%(type)s(%(c_type)s obj) +void qapi_free_%(type)s(%(c_type)sobj) { QapiDeallocVisitor *md; Visitor *v; diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 45ce3a9..8ffed3d 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -38,7 +38,7 @@ def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, base = ret += mcgen(''' -static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error **errp) +static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s **obj, Error **errp) { Error *err = NULL; ''', @@ -149,7 +149,7 @@ def generate_visit_struct(expr): ret += mcgen(''' -void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp) +void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp) { ''', name=name) @@ -166,7 +166,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** def generate_visit_list(name, members): return mcgen(''' -void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp) +void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp) { GenericList *i, **prev = (GenericList **)obj; Error *err = NULL; @@ -193,7 +193,7 @@ void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, def generate_visit_enum(name, members): return mcgen(''' -void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **errp) +void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **errp) { visit_type_enum(m, (int *)obj, %(name)s_lookup, "%(name)s", name, errp); } @@ -203,7 +203,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **e def generate_visit_anon_union(name, members): ret = mcgen(''' -void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp) +void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp) { Error *err = NULL; @@ -279,7 +279,7 @@ def generate_visit_union(expr): ret += mcgen(''' -void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp) +void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp) { Error *err = NULL; @@ -367,13 +367,13 @@ def generate_declaration(name, members, genlist=True, builtin_type=False): if not builtin_type: ret += mcgen(''' -void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp); +void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp); ''', name=name) if genlist: ret += mcgen(''' -void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp); +void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); ''', name=name) @@ -383,7 +383,7 @@ def generate_enum_declaration(name, members, genlist=True): ret = "" if genlist: ret += mcgen(''' -void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp); +void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); ''', name=name) @@ -392,7 +392,7 @@ void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, def generate_decl_enum(name, members, genlist=True): return mcgen(''' -void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **errp); +void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **errp); ''', name=name) diff --git a/scripts/qapi.py b/scripts/qapi.py index b474c39..421f94f 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -425,23 +425,23 @@ def c_type(name): if name == 'str': return 'char *' elif name == 'int': - return 'int64_t' + return 'int64_t ' elif (name == 'int8' or name == 'int16' or name == 'int32' or name == 'int64' or name == 'uint8' or name == 'uint16' or name == 'uint32' or name == 'uint64'): - return name + '_t' + return name + '_t ' elif name == 'size': - return 'uint64_t' + return 'uint64_t ' elif name == 'bool': - return 'bool' + return 'bool ' elif name == 'number': - return 'double' + return 'double ' elif type(name) == list: return '%s *' % c_list_type(name[0]) elif is_enum(name): - return name + return name + ' ' elif name == None or len(name) == 0: - return 'void' + return 'void ' elif name == name.upper(): return '%sEvent *' % camel_case(name) else:
Currently we always add a space after c_type in mcgen(), there is some redundant space in generated code. The space isn't needed for points by the coding style. char * value; ^ qapi_free_NameInfo(NameInfo * obj) ^ It's fussy to add checking in each mcgen(), this patch just addes the necessary space in c_type(), and remove original space in mcgen(). Signed-off-by: Amos Kong <akong@redhat.com> --- scripts/qapi-types.py | 10 +++++----- scripts/qapi-visit.py | 20 ++++++++++---------- scripts/qapi.py | 14 +++++++------- 3 files changed, 22 insertions(+), 22 deletions(-)