[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 <akong@redhat.com>
---
 scripts/qapi-commands.py |  2 +-
 scripts/qapi.py          | 11 ++++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

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 <akong@redhat.com>
> ---
>  scripts/qapi-commands.py |  2 +-
>  scripts/qapi.py          | 11 ++++++-----
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

Amos Kong <akong@redhat.com> 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 <akong@redhat.com>
> ---
>  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)

On Fri, Apr 25, 2014 at 02:52:09PM +0200, Markus Armbruster wrote:
> Amos Kong <akong@redhat.com> 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 <akong@redhat.com>
> > ---
> >  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 Kong <akong@redhat.com> writes:

> On Fri, Apr 25, 2014 at 02:52:09PM +0200, Markus Armbruster wrote:
>> Amos Kong <akong@redhat.com> 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 <akong@redhat.com>
>> > ---
>> >  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.

[...]