[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/2] qapi: add a special string in c_type()'s
From: |
Amos Kong |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/2] qapi: add a special string in c_type()'s result to each redundant space |
Date: |
Mon, 28 Apr 2014 14:57:06 +0800 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Fri, Apr 25, 2014 at 02:52:09PM +0200, Markus Armbruster wrote:
> Amos Kong <address@hidden> 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 <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 :)
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.