Re: [Qemu-devel] [PATCH v2 2/2] qapi: add a special string in c_type()'s result to each redundant space
Amos Kong writes: > On Fri, Apr 25, 2014 at 02:52:09PM +0200, Markus Armbruster wrote: >> Amos Kong writes: > > Hi Markus, > >> > 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. >> >> You mean pointers. >> >> > >> > char * value; >> > ^ >> > qapi_free_NameInfo(NameInfo * obj) >> >^ >> > >> > This patch added a special string 'EATSPACE' in the end of c_type()'s >> > result only for point type. The string and the following space will be >> > striped in mcgen(). >> >> Suggest: >> >> qapi: Suppress unwanted space between type and identifier >> >> We always generate a space between type and identifier in parameter >> and variable declarations, even when idiomatic C style doesn't have >> a space there. Suppress it. >> >> This explains what you do and why, but not how. A commit message should >> always cover "what" and "why", but "how" can be ommitted in simple cases >> like this one. >> >> Use of "EATSPACE" as marker is unnecessarily fragile, as it could >> legitimately occur in generated code. Recommend to pick something that >> can't, such as "\033EATSPACE." > > "\033EATSPACE." works, I will use it. > >> > Signed-off-by: Amos Kong >> > --- >> > scripts/qapi-commands.py | 2 +- >> > scripts/qapi.py | 11 ++- >> > 2 files changed, 7 insertions(+), 6 deletions(-) >> > >> > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py >> > index 9734ab0..0ebb1b9 100644 >> > --- a/scripts/qapi-commands.py >> > +++ b/scripts/qapi-commands.py >> > @@ -26,7 +26,7 @@ def type_visitor(name): >> > def generate_command_decl(name, args, ret_type): >> > arglist="" >> > for argname, argtype, optional, structured in parse_args(args): >> > -argtype = c_type(argtype) >> > +argtype = c_type(argtype).replace('EATSPACE', '') >> > if argtype == "char *": >> > argtype = "const char *" >> > if optional: >> >> Ugly, and you make it uglier :) > > It's not just ugly, eatspace string is cleaned, so the space can be > eaten. I was too terse again, sorry. I mean: generate_command_decl() messing with the result of c_type() like this is ugly, because it requires it to know exactly what c_type() does. [...]
Re: [Qemu-devel] [PATCH v2 2/2] qapi: add a special string in c_type()'s result to each redundant space
On Fri, Apr 25, 2014 at 02:52:09PM +0200, Markus Armbruster wrote: > Amos Kong writes: Hi Markus, > > 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. > > You mean pointers. > > > > > char * value; > > ^ > > qapi_free_NameInfo(NameInfo * obj) > >^ > > > > This patch added a special string 'EATSPACE' in the end of c_type()'s > > result only for point type. The string and the following space will be > > striped in mcgen(). > > Suggest: > > qapi: Suppress unwanted space between type and identifier > > We always generate a space between type and identifier in parameter > and variable declarations, even when idiomatic C style doesn't have > a space there. Suppress it. > > This explains what you do and why, but not how. A commit message should > always cover "what" and "why", but "how" can be ommitted in simple cases > like this one. > > Use of "EATSPACE" as marker is unnecessarily fragile, as it could > legitimately occur in generated code. Recommend to pick something that > can't, such as "\033EATSPACE." "\033EATSPACE." works, I will use it. > > Signed-off-by: Amos Kong > > --- > > scripts/qapi-commands.py | 2 +- > > scripts/qapi.py | 11 ++- > > 2 files changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > > index 9734ab0..0ebb1b9 100644 > > --- a/scripts/qapi-commands.py > > +++ b/scripts/qapi-commands.py > > @@ -26,7 +26,7 @@ def type_visitor(name): > > def generate_command_decl(name, args, ret_type): > > arglist="" > > for argname, argtype, optional, structured in parse_args(args): > > -argtype = c_type(argtype) > > +argtype = c_type(argtype).replace('EATSPACE', '') > > if argtype == "char *": > > argtype = "const char *" > > if optional: > > Ugly, and you make it uglier :) It's not just ugly, eatspace string is cleaned, so the space can be eaten. > Two cleaner alternatives: > > * Test argname instead. Still ugly, but less so. > > * Make c_type() take optional parameter is_param, and have it add const > when true. I will add a parameter to only add 'const ' prefix for str type. > > diff --git a/scripts/qapi.py b/scripts/qapi.py > > index b474c39..b46c518 100644 > > --- a/scripts/qapi.py > > +++ b/scripts/qapi.py > > @@ -423,7 +423,7 @@ def is_enum(name): > > > > def c_type(name): > > if name == 'str': > > -return 'char *' > > +return 'char *EATSPACE' > > elif name == 'int': > > return 'int64_t' > > elif (name == 'int8' or name == 'int16' or name == 'int32' or > > @@ -437,15 +437,15 @@ def c_type(name): > > elif name == 'number': > > return 'double' > > elif type(name) == list: > > -return '%s *' % c_list_type(name[0]) > > +return '%s *EATSPACE' % c_list_type(name[0]) > > elif is_enum(name): > > return name > > elif name == None or len(name) == 0: > > return 'void' > > elif name == name.upper(): > > -return '%sEvent *' % camel_case(name) > > +return '%sEvent *EATSPACE' % camel_case(name) > > else: > > -return '%s *' % name > > +return '%s *EATSPACE' % name > > > > def genindent(count): > > ret = "" > > Please define a global constant instead of repeating your magic string > over and over. > > > @@ -470,7 +470,8 @@ def cgen(code, **kwds): > > return '\n'.join(lines) % kwds + '\n' > > > > def mcgen(code, **kwds): > > -return cgen('\n'.join(code.split('\n')[1:-1]), **kwds) > > +raw = cgen('\n'.join(code.split('\n')[1:-1]), **kwds) > > +return raw.replace('*EATSPACE ', '*') > > > > def basename(filename): > > return filename.split("/")[-1] > > This makes EATSPACE work only after '*'. You never emit it anywhere > else, but relying on it here isn't necessary. I'd do something like > > return re.sub(re.escape(eatspace) + ' +', '', raw) OK Thanks. -- Amos.
Re: [Qemu-devel] [PATCH v2 2/2] qapi: add a special string in c_type()'s result to each redundant space
Amos Kong writes: > 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. You mean pointers. > > char * value; > ^ > qapi_free_NameInfo(NameInfo * obj) >^ > > This patch added a special string 'EATSPACE' in the end of c_type()'s > result only for point type. The string and the following space will be > striped in mcgen(). Suggest: qapi: Suppress unwanted space between type and identifier We always generate a space between type and identifier in parameter and variable declarations, even when idiomatic C style doesn't have a space there. Suppress it. This explains what you do and why, but not how. A commit message should always cover "what" and "why", but "how" can be ommitted in simple cases like this one. Use of "EATSPACE" as marker is unnecessarily fragile, as it could legitimately occur in generated code. Recommend to pick something that can't, such as "\033EATSPACE." > Signed-off-by: Amos Kong > --- > scripts/qapi-commands.py | 2 +- > scripts/qapi.py | 11 ++- > 2 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index 9734ab0..0ebb1b9 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > @@ -26,7 +26,7 @@ def type_visitor(name): > def generate_command_decl(name, args, ret_type): > arglist="" > for argname, argtype, optional, structured in parse_args(args): > -argtype = c_type(argtype) > +argtype = c_type(argtype).replace('EATSPACE', '') > if argtype == "char *": > argtype = "const char *" > if optional: Ugly, and you make it uglier :) Two cleaner alternatives: * Test argname instead. Still ugly, but less so. * Make c_type() take optional parameter is_param, and have it add const when true. > diff --git a/scripts/qapi.py b/scripts/qapi.py > index b474c39..b46c518 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -423,7 +423,7 @@ def is_enum(name): > > def c_type(name): > if name == 'str': > -return 'char *' > +return 'char *EATSPACE' > elif name == 'int': > return 'int64_t' > elif (name == 'int8' or name == 'int16' or name == 'int32' or > @@ -437,15 +437,15 @@ def c_type(name): > elif name == 'number': > return 'double' > elif type(name) == list: > -return '%s *' % c_list_type(name[0]) > +return '%s *EATSPACE' % c_list_type(name[0]) > elif is_enum(name): > return name > elif name == None or len(name) == 0: > return 'void' > elif name == name.upper(): > -return '%sEvent *' % camel_case(name) > +return '%sEvent *EATSPACE' % camel_case(name) > else: > -return '%s *' % name > +return '%s *EATSPACE' % name > > def genindent(count): > ret = "" Please define a global constant instead of repeating your magic string over and over. > @@ -470,7 +470,8 @@ def cgen(code, **kwds): > return '\n'.join(lines) % kwds + '\n' > > def mcgen(code, **kwds): > -return cgen('\n'.join(code.split('\n')[1:-1]), **kwds) > +raw = cgen('\n'.join(code.split('\n')[1:-1]), **kwds) > +return raw.replace('*EATSPACE ', '*') > > def basename(filename): > return filename.split("/")[-1] This makes EATSPACE work only after '*'. You never emit it anywhere else, but relying on it here isn't necessary. I'd do something like return re.sub(re.escape(eatspace) + ' +', '', raw)
Re: [Qemu-devel] [PATCH v2 2/2] qapi: add a special string in c_type()'s result to each redundant space
On 04/25/2014 01:56 AM, 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. s/points/pointers/ > > char * value; > ^ > qapi_free_NameInfo(NameInfo * obj) >^ > > This patch added a special string 'EATSPACE' in the end of c_type()'s Present tense for what a patch does: s/added/adds/ > result only for point type. The string and the following space will be s/point type/pointer types/ > striped in mcgen(). s/striped/stripped/ > > Signed-off-by: Amos Kong > --- > scripts/qapi-commands.py | 2 +- > scripts/qapi.py | 11 ++- > 2 files changed, 7 insertions(+), 6 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH v2 2/2] qapi: add a special string in c_type()'s result to each redundant space
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) ^ This patch added a special string 'EATSPACE' in the end of c_type()'s result only for point type. The string and the following space will be striped in mcgen(). Signed-off-by: Amos Kong --- scripts/qapi-commands.py | 2 +- scripts/qapi.py | 11 ++- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 9734ab0..0ebb1b9 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -26,7 +26,7 @@ def type_visitor(name): def generate_command_decl(name, args, ret_type): arglist="" for argname, argtype, optional, structured in parse_args(args): -argtype = c_type(argtype) +argtype = c_type(argtype).replace('EATSPACE', '') if argtype == "char *": argtype = "const char *" if optional: diff --git a/scripts/qapi.py b/scripts/qapi.py index b474c39..b46c518 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -423,7 +423,7 @@ def is_enum(name): def c_type(name): if name == 'str': -return 'char *' +return 'char *EATSPACE' elif name == 'int': return 'int64_t' elif (name == 'int8' or name == 'int16' or name == 'int32' or @@ -437,15 +437,15 @@ def c_type(name): elif name == 'number': return 'double' elif type(name) == list: -return '%s *' % c_list_type(name[0]) +return '%s *EATSPACE' % c_list_type(name[0]) elif is_enum(name): return name elif name == None or len(name) == 0: return 'void' elif name == name.upper(): -return '%sEvent *' % camel_case(name) +return '%sEvent *EATSPACE' % camel_case(name) else: -return '%s *' % name +return '%s *EATSPACE' % name def genindent(count): ret = "" @@ -470,7 +470,8 @@ def cgen(code, **kwds): return '\n'.join(lines) % kwds + '\n' def mcgen(code, **kwds): -return cgen('\n'.join(code.split('\n')[1:-1]), **kwds) +raw = cgen('\n'.join(code.split('\n')[1:-1]), **kwds) +return raw.replace('*EATSPACE ', '*') def basename(filename): return filename.split("/")[-1] -- 1.9.0