| Message ID | 1400762504-22751-4-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 05/22/2014 06:41 AM, Amos Kong wrote: > 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 <akong@redhat.com> > --- > scripts/qapi-commands.py | 2 +- > scripts/qapi.py | 19 +++++++++++++------ > 2 files changed, 14 insertions(+), 7 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
Amos Kong <akong@redhat.com> 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 <akong@redhat.com> > --- > 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. 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) 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.
On Mon, 02 Jun 2014 17:43:58 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Amos Kong <akong@redhat.com> 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 <akong@redhat.com> > > --- > > 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. > > 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) > > Don't you have to patch that, too? Amos, have you answered this comment already? This series is pending on this. > > > 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. >
On Mon, Jun 02, 2014 at 05:43:58PM +0200, Markus Armbruster wrote: > Amos Kong <akong@redhat.com> 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 <akong@redhat.com> > > --- > > 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 :( [amos@z 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 Kong <akong@redhat.com> writes: > On Mon, Jun 02, 2014 at 05:43:58PM +0200, Markus Armbruster wrote: >> Amos Kong <akong@redhat.com> 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 <akong@redhat.com> >> > --- >> > 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) Either that, or a suitable conditional matching (relevant parts of) c_type(). Your choice. >> 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 :( That's why we do reviews :) [...]
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; ''', 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]
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 <akong@redhat.com> --- scripts/qapi-commands.py | 2 +- scripts/qapi.py | 19 +++++++++++++------ 2 files changed, 14 insertions(+), 7 deletions(-)