[Qemu-devel] [PATCH v6 0/3] qapi: fix coding style in generated code

[Qemu-devel] [PATCH v6 0/3] qapi: fix coding style in generated code

[Amos Kong]

Not a serious issue, but it's helpful if we can fix it.

V2: split change of scripts/qapi-visit.py to a split patch,
    eat space by using a special char as Markus suggested
V3: update commitlog, update special string, fix of adding
    const replace string by pattern
V4: fix pattern to cleanup special string (Paolo)
V5: fix string checking bug (Luiz), update commitlog (Eric)
    add comments for c_type() function
V6: add is_c_ptr() and fix lost endswith (Markus)

Amos Kong (3):
  qapi: fix coding style in parameters list
  qapi: add const prefix to 'char *' insider c_type()
  qapi: Suppress unwanted space between type and identifier

 scripts/qapi-commands.py |  8 +++-----
 scripts/qapi-visit.py    | 20 ++++++++++----------
 scripts/qapi.py          | 25 +++++++++++++++++++------
 3 files changed, 32 insertions(+), 21 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v6 1/3] qapi: fix coding style in parameters list

[Amos Kong]

A space after * when declaring a pointer type is redundant.

Signed-off-by: Amos Kong <akong@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-visit.py | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 06a79f1..c129697 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -77,7 +77,7 @@ static void visit_type_%(full_name)s_field_%(c_name)s(Visitor *m, %(name)s **obj
 
     ret += mcgen('''
 
-static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error **errp)
+static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s **obj, Error **errp)
 {
     Error *err = NULL;
 ''',
@@ -186,7 +186,7 @@ def generate_visit_struct(expr):
 
     ret += mcgen('''
 
-void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
+void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp)
 {
 ''',
                 name=name)
@@ -201,7 +201,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
 def generate_visit_list(name, members):
     return mcgen('''
 
-void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
+void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp)
 {
     Error *err = NULL;
     GenericList *i, **prev;
@@ -230,7 +230,7 @@ out:
 def generate_visit_enum(name, members):
     return mcgen('''
 
-void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **errp)
+void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **errp)
 {
     visit_type_enum(m, (int *)obj, %(name)s_lookup, "%(name)s", name, errp);
 }
@@ -240,7 +240,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **e
 def generate_visit_anon_union(name, members):
     ret = mcgen('''
 
-void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
+void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;
 
@@ -327,7 +327,7 @@ def generate_visit_union(expr):
 
     ret += mcgen('''
 
-void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
+void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;
 
@@ -399,13 +399,13 @@ def generate_declaration(name, members, genlist=True, builtin_type=False):
     if not builtin_type:
         ret += mcgen('''
 
-void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp);
+void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp);
 ''',
                     name=name)
 
     if genlist:
         ret += mcgen('''
-void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp);
+void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp);
 ''',
                  name=name)
 
@@ -415,7 +415,7 @@ def generate_enum_declaration(name, members, genlist=True):
     ret = ""
     if genlist:
         ret += mcgen('''
-void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp);
+void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp);
 ''',
                      name=name)
 
@@ -424,7 +424,7 @@ void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name,
 def generate_decl_enum(name, members, genlist=True):
     return mcgen('''
 
-void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **errp);
+void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **errp);
 ''',
                 name=name)
 
-- 
1.9.3

[Qemu-devel] [PATCH v6 2/3] qapi: add const prefix to 'char *' insider c_type()

[Amos Kong]

It's ugly to add const prefix for parameter type by an if statement
outside c_type(). This patch adds a parameter to do it.

Signed-off-by: Amos Kong <akong@redhat.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-commands.py | 4 +---
 scripts/qapi.py          | 4 +++-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 7d93d01..34f200a 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -29,9 +29,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)
-        if argtype == "char *":
-            argtype = "const char *"
+        argtype = c_type(argtype, is_param=True)
         if optional:
             arglist += "bool has_%s, " % c_var(argname)
         arglist += "%s %s, " % (argtype, c_var(argname))
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 86e9608..dc690bb 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -470,8 +470,10 @@ def find_enum(name):
 def is_enum(name):
     return find_enum(name) != None
 
-def c_type(name):
+def c_type(name, is_param=False):
     if name == 'str':
+        if is_param:
+            return 'const char *'
         return 'char *'
     elif name == 'int':
         return 'int64_t'
-- 
1.9.3

Re: [Qemu-devel] [PATCH v6 2/3] qapi: add const prefix to 'char *' insider c_type()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]

On 06/10/2014 05:25 AM, Amos Kong wrote:
> It's ugly to add const prefix for parameter type by an if statement
> outside c_type(). This patch adds a parameter to do it.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-commands.py | 4 +---
>  scripts/qapi.py          | 4 +++-
>  2 files changed, 4 insertions(+), 4 deletions(-)

Wenchao's series introduces another client that needs this treatment:
https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg01225.html

Depending on what order things get merged in, you may need followup
patches or conflict resolution.

> 
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 7d93d01..34f200a 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -29,9 +29,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)
> -        if argtype == "char *":
> -            argtype = "const char *"
> +        argtype = c_type(argtype, is_param=True)

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v6 2/3] qapi: add const prefix to 'char *' insider c_type()

[Amos Kong]

[-- Attachment #1: Type: text/plain, Size: 1770 bytes --]

On Tue, Jun 17, 2014 at 09:51:21PM -0600, Eric Blake wrote:
> On 06/10/2014 05:25 AM, Amos Kong wrote:
> > It's ugly to add const prefix for parameter type by an if statement
> > outside c_type(). This patch adds a parameter to do it.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >  scripts/qapi-commands.py | 4 +---
> >  scripts/qapi.py          | 4 +++-
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> Wenchao's series introduces another client that needs this treatment:
> https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg01225.html
> 
> Depending on what order things get merged in, you may need followup
> patches or conflict resolution.

Thanks for the reminder.

I just checked the patch, c_type() is only used once, and the output
is used insider mcgen().

So it's safe to apply my patchset.

> > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> > index 7d93d01..34f200a 100644
> > --- a/scripts/qapi-commands.py
> > +++ b/scripts/qapi-commands.py
> > @@ -29,9 +29,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)
> > -        if argtype == "char *":
> > -            argtype = "const char *"
> > +        argtype = c_type(argtype, is_param=True)
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 



-- 
			Amos.

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Amos Kong]

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 |  4 ++--
 scripts/qapi.py          | 23 +++++++++++++++++------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 34f200a..053ba85 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 is_c_ptr(argtype):
             ret += mcgen('''
 %(argtype)s %(argname)s = NULL;
 ''',
@@ -227,7 +227,7 @@ def gen_marshal_input(name, args, ret_type, middle_mode):
 ''')
 
     if ret_type:
-        if c_type(ret_type).endswith("*"):
+        if is_c_ptr(ret_type):
             retval = "    %s retval = NULL;" % c_type(ret_type)
         else:
             retval = "    %s retval;" % c_type(ret_type)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index dc690bb..0079194 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,19 @@ 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 is_c_ptr(name):
+    suffix = "*" + eatspace
+    return c_type(name).endswith(suffix)
 
 def genindent(count):
     ret = ""
@@ -521,7 +531,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]
-- 
1.9.3

Re: [Qemu-devel] [PATCH v6 0/3] qapi: fix coding style in generated code

[Luiz Capitulino]

On Tue, 10 Jun 2014 19:25:50 +0800
Amos Kong <akong@redhat.com> wrote:

> Not a serious issue, but it's helpful if we can fix it.

Applied to the qmp branch, thanks.

> 
> V2: split change of scripts/qapi-visit.py to a split patch,
>     eat space by using a special char as Markus suggested
> V3: update commitlog, update special string, fix of adding
>     const replace string by pattern
> V4: fix pattern to cleanup special string (Paolo)
> V5: fix string checking bug (Luiz), update commitlog (Eric)
>     add comments for c_type() function
> V6: add is_c_ptr() and fix lost endswith (Markus)
> 
> Amos Kong (3):
>   qapi: fix coding style in parameters list
>   qapi: add const prefix to 'char *' insider c_type()
>   qapi: Suppress unwanted space between type and identifier
> 
>  scripts/qapi-commands.py |  8 +++-----
>  scripts/qapi-visit.py    | 20 ++++++++++----------
>  scripts/qapi.py          | 25 +++++++++++++++++++------
>  3 files changed, 32 insertions(+), 21 deletions(-)
>