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

2014-04-28 Thread Amos Kong 
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.

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

2014-04-28 Thread Markus Armbruster 
Amos Kong ak...@redhat.com writes:

 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.

I was too terse again, sorry.  I mean: generate_command_decl() messing
with the result of c_type() like this is ugly, because it requires it to
know exactly what c_type() does.

[...]

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

2014-04-25 Thread Amos Kong 
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.

  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().

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:
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 = 
@@ -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]
-- 
1.9.0

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

2014-04-25 Thread Eric Blake 
On 04/25/2014 01:56 AM, Amos Kong wrote:
 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.

s/points/pointers/

 
   char * value;
 ^
   qapi_free_NameInfo(NameInfo * obj)
^
 
 This patch added a special string 'EATSPACE' in the end of c_type()'s

Present tense for what a patch does: s/added/adds/

 result only for point type. The string and the following space will be

s/point type/pointer types/

 striped in mcgen().

s/striped/stripped/

 
 Signed-off-by: Amos Kong ak...@redhat.com
 ---
  scripts/qapi-commands.py |  2 +-
  scripts/qapi.py  | 11 ++-
  2 files changed, 7 insertions(+), 6 deletions(-)
 

Reviewed-by: Eric Blake ebl...@redhat.com

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

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

2014-04-25 Thread Markus Armbruster 
Amos Kong ak...@redhat.com 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 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 :)

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)