Re: [Qemu-devel] [PATCH v5 3/3] qapi: Suppress unwanted space between type and identifier

[Amos Kong]

On Mon, Jun 02, 2014 at 05:43:58PM +0200, Markus Armbruster wrote:
> Amos Kong <address@hidden> writes:
> 
> > 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.
> >
> > Signed-off-by: Amos Kong <address@hidden>
> > ---
> >  scripts/qapi-commands.py |  2 +-
> >  scripts/qapi.py          | 19 +++++++++++++------
> >  2 files changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> > index 34f200a..4b49735 100644
> > --- a/scripts/qapi-commands.py
> > +++ b/scripts/qapi-commands.py
> > @@ -102,7 +102,7 @@ def gen_visitor_input_vars_decl(args):
> >  bool has_%(argname)s = false;
> >  ''',
> >                           argname=c_var(argname))
> > -        if c_type(argtype).endswith("*"):
> > +        if c_type(argtype).endswith("*" + eatspace):
> >              ret += mcgen('''
> >  %(argtype)s %(argname)s = NULL;
> >  ''',
> 
> Ugly.  A function is_c_ptr(argtype) would be cleaner.

def is_c_ptr(argtype):
    suffix = "*" + eatspace
    return c_type(argtype).endswith(suffix)
 
> There's similar code in gen_marshal_input():
> 
>     if ret_type:
>         if c_type(ret_type).endswith("*"):
>             retval = "    %s retval = NULL;" % c_type(ret_type)
>         else:
>             retval = "    %s retval;" % c_type(ret_type)
>         ret += mcgen('''
> %(retval)s
> ''',
>                      retval=retval)

Thanks, I searched by grep in the past, but I still lost one :( 

address@hidden qemu]$ git grep c_type|grep endswith
scripts/qapi-commands.py:        if c_type(argtype).endswith("*"):
scripts/qapi-commands.py:        if c_type(ret_type).endswith("*"):
 
> Don't you have to patch that, too?
> 
> > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > index dc690bb..562d560 100644
> > --- a/scripts/qapi.py
> > +++ b/scripts/qapi.py
> > @@ -470,11 +470,17 @@ def find_enum(name):
> >  def is_enum(name):
> >      return find_enum(name) != None
> >  
> > +eatspace = '\033EATSPACE.'
> > +
> > +# A special suffix is added in c_type() for pointer types, and it's
> > +# stripped in mcgen(). So please notice this when you check the return
> > +# value of c_type() outside mcgen().
> >  def c_type(name, is_param=False):
> >      if name == 'str':
> >          if is_param:
> > -            return 'const char *'
> > -        return 'char *'
> > +            return 'const char *' + eatspace
> > +        return 'char *' + eatspace
> > +
> >      elif name == 'int':
> >          return 'int64_t'
> >      elif (name == 'int8' or name == 'int16' or name == 'int32' or
> > @@ -488,15 +494,15 @@ def c_type(name, is_param=False):
> >      elif name == 'number':
> >          return 'double'
> >      elif type(name) == list:
> > -        return '%s *' % c_list_type(name[0])
> > +        return '%s *%s' % (c_list_type(name[0]), eatspace)
> >      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 *%s' % (camel_case(name), eatspace)
> >      else:
> > -        return '%s *' % name
> > +        return '%s *%s' % (name, eatspace)
> >  
> >  def genindent(count):
> >      ret = ""
> > @@ -521,7 +527,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 re.sub(re.escape(eatspace) + ' *', '', raw)
> >  
> >  def basename(filename):
> >      return filename.split("/")[-1]
> 
> Rest looks good.

-- 
                        Amos.