Re: [Qemu-devel] [PATCH v2 2/2] qapi: add a special string in c_type()'s result to each redundant space

[Markus Armbruster]

Amos Kong <address@hidden> 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 <address@hidden>
> ---
>  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)