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