[Qemu-devel] [PATCH v4 16/16] qapi: Prefer '"str" + var' over '"str%s" % var'

[Eric Blake <eblake@redhat.com>]

Use of python's % operator to format strings is fine if there are
multiple strings or if there is integer formatting going on, but
when it is just abused for string concatenation, it looks nicer
to just use the + operator.  This is particularly true when the
value being substituted is at the front of the format string,
rather than the tail.

Update an error message (and testsuite coverage) while at it, since
concatenating a non-string object does not always produce legible
results.

Signed-off-by: Eric Blake <eblake@redhat.com>

---

v4: new patch
---
 scripts/qapi-commands.py               | 17 +++++----
 scripts/qapi-event.py                  |  2 +-
 scripts/qapi-types.py                  | 10 +++---
 scripts/qapi-visit.py                  |  4 +--
 scripts/qapi.py                        | 64 +++++++++++++++++-----------------
 tests/qapi-schema/include-non-file.err |  2 +-
 6 files changed, 51 insertions(+), 48 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 0a1d636..1464a3d 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -25,7 +25,7 @@ def generate_command_decl(name, args, ret_type):
     for argname, argtype, optional in parse_args(args):
         argtype = c_type(argtype, is_param=True)
         if optional:
-            arglist += "bool has_%s, " % c_name(argname)
+            arglist += "bool has_" + c_name(argname) + ", "
         arglist += "%s %s, " % (argtype, c_name(argname))
     return mcgen('''
 %(ret_type)s qmp_%(name)s(%(args)sError **errp);
@@ -50,8 +50,8 @@ def gen_sync_call(name, args, ret_type, indent=0):
         retval = "retval = "
     for argname, argtype, optional in parse_args(args):
         if optional:
-            arglist += "has_%s, " % c_name(argname)
-        arglist += "%s, " % (c_name(argname))
+            arglist += "has_" + c_name(argname) + ", "
+        arglist += c_name(argname) + ", "
     push_indent(indent)
     ret = mcgen('''
 %(retval)sqmp_%(name)s(%(args)s&local_err);
@@ -71,7 +71,7 @@ def gen_sync_call(name, args, ret_type, indent=0):
 def gen_marshal_output_call(name, ret_type):
     if not ret_type:
         return ""
-    return "qmp_marshal_output_%s(retval, ret, &local_err);" % c_name(name)
+    return "qmp_marshal_output_" + c_name(name) + "(retval, ret, &local_err);"

 def gen_visitor_input_containers_decl(args, obj):
     ret = ""
@@ -200,9 +200,11 @@ out:

 def gen_marshal_input_decl(name, args, ret_type, middle_mode):
     if middle_mode:
-        return 'int qmp_marshal_input_%s(Monitor *mon, const QDict *qdict, QObject **ret)' % c_name(name)
+        return ('int qmp_marshal_input_' + c_name(name)
+                + '(Monitor *mon, const QDict *qdict, QObject **ret)')
     else:
-        return 'static void qmp_marshal_input_%s(QDict *args, QObject **ret, Error **errp)' % c_name(name)
+        return ('static void qmp_marshal_input_' + c_name(name)
+                + '(QDict *args, QObject **ret, Error **errp)')



@@ -458,7 +460,8 @@ if dispatch_type == "sync":
             fdef.write(ret)

         if middle_mode:
-            fdecl.write('%s;\n' % gen_marshal_input_decl(cmd['command'], arglist, ret_type, middle_mode))
+            fdecl.write(gen_marshal_input_decl(cmd['command'], arglist,
+                                               ret_type, middle_mode) + ';\n')

         ret = gen_marshal_input(cmd['command'], arglist, ret_type, middle_mode) + "\n"
         fdef.write(ret)
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index a7e0033..957dba5 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -17,7 +17,7 @@ import getopt
 import errno

 def _generate_event_api_name(event_name, params):
-    api_name = "void qapi_event_send_%s(" % c_name(event_name).lower();
+    api_name = "void qapi_event_send_" + c_name(event_name).lower() + "(";
     l = len(api_name)

     if params:
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 5665145..1c41743 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -204,7 +204,7 @@ def generate_union(expr, meta):
     if enum_define:
         discriminator_type_name = enum_define['enum_name']
     else:
-        discriminator_type_name = '%sKind' % (name)
+        discriminator_type_name = name + 'Kind'

     ret = mcgen('''
 struct %(name)s
@@ -399,13 +399,13 @@ for expr in exprs:
         ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
         enum_define = discriminator_find_enum_define(expr)
         if not enum_define:
-            ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
-            fdef.write(generate_enum_lookup('%sKind' % expr['union'],
+            ret += generate_enum(expr['union'] + 'Kind', expr['data'].keys())
+            fdef.write(generate_enum_lookup(expr['union'] + 'Kind',
                                             expr['data'].keys()))
     elif expr.has_key('alternate'):
         ret += generate_fwd_struct(expr['alternate'], expr['data']) + "\n"
-        ret += generate_enum('%sKind' % expr['alternate'], expr['data'].keys())
-        fdef.write(generate_enum_lookup('%sKind' % expr['alternate'],
+        ret += generate_enum(expr['alternate'] + 'Kind', expr['data'].keys())
+        fdef.write(generate_enum_lookup(expr['alternate'] + 'Kind',
                                         expr['data'].keys()))
         fdef.write(generate_alternate_qtypes(expr))
     else:
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index e511be3..4287251 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -509,7 +509,7 @@ for expr in exprs:
         enum_define = discriminator_find_enum_define(expr)
         ret = ""
         if not enum_define:
-            ret = generate_decl_enum('%sKind' % expr['union'],
+            ret = generate_decl_enum(expr['union'] + 'Kind',
                                      expr['data'].keys())
         ret += generate_declaration(expr['union'], expr['data'])
         fdecl.write(ret)
@@ -518,7 +518,7 @@ for expr in exprs:
         ret += generate_visit_list(expr['alternate'], expr['data'])
         fdef.write(ret)

-        ret = generate_decl_enum('%sKind' % expr['alternate'],
+        ret = generate_decl_enum(expr['alternate'] + 'Kind',
                                  expr['data'].keys())
         ret += generate_declaration(expr['alternate'], expr['data'])
         fdecl.write(ret)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 309dfec..1a11f61 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -122,21 +122,22 @@ class QAPISchema:
         self.accept()

         while self.tok != None:
-            expr_info = {'file': input_relname, 'line': self.line, 'parent': self.parent_info}
+            expr_info = {'file': input_relname, 'line': self.line,
+                         'parent': self.parent_info}
             expr = self.get_expr(False)
             if isinstance(expr, dict) and "include" in expr:
                 if len(expr) != 1:
-                    raise QAPIExprError(expr_info, "Invalid 'include' directive")
+                    raise QAPIExprError(expr_info,
+                                        "Invalid 'include' directive")
                 include = expr["include"]
                 if not isinstance(include, str):
                     raise QAPIExprError(expr_info,
-                                        'Expected a file name (string), got: %s'
-                                        % include)
+                                        "Expected a string for 'include' value")
                 include_path = os.path.join(self.input_dir, include)
                 for elem in self.include_hist:
                     if include_path == elem[1]:
-                        raise QAPIExprError(expr_info, "Inclusion loop for %s"
-                                            % include)
+                        raise QAPIExprError(expr_info,
+                                            "Inclusion loop for " + include)
                 # skip multiple include of the same file
                 if include_path in previously_included:
                     continue
@@ -208,7 +209,7 @@ class QAPISchema:
                             string += ch
                         else:
                             raise QAPISchemaError(self,
-                                                  "Unknown escape \\%s" %ch)
+                                                  "Unknown escape \\" + ch)
                         esc = False
                     elif ch == "\\":
                         esc = True
@@ -254,7 +255,7 @@ class QAPISchema:
                 raise QAPISchemaError(self, 'Expected ":"')
             self.accept()
             if key in expr:
-                raise QAPISchemaError(self, 'Duplicate key "%s"' % key)
+                raise QAPISchemaError(self, 'Duplicate key "' + key + '"')
             expr[key] = self.get_expr(True)
             if self.tok == '}':
                 self.accept()
@@ -343,7 +344,7 @@ def check_name(expr_info, source, name, allow_optional = False,

     if not isinstance(name, str):
         raise QAPIExprError(expr_info,
-                            "%s requires a string name" % source)
+                            source + " requires a string name")
     if name.startswith('*'):
         membername = name[1:]
         if not allow_optional:
@@ -374,13 +375,12 @@ def check_type(expr_info, source, value, allow_array = False,
     if isinstance(value, list):
         if not allow_array:
             raise QAPIExprError(expr_info,
-                                "%s cannot be an array" % source)
+                                source + " cannot be an array")
         if len(value) != 1 or not isinstance(value[0], str):
-            raise QAPIExprError(expr_info,
-                                "%s: array type must contain single type name"
-                                % source)
+            raise QAPIExprError(expr_info, source +
+                                ": array type must contain single type name")
         value = value[0]
-        orig_value = "array of %s" %value
+        orig_value = "array of " + value

     # Check if type name for value is okay
     if isinstance(value, str):
@@ -401,12 +401,12 @@ def check_type(expr_info, source, value, allow_array = False,
     # value is a dictionary, check that each member is okay
     if not isinstance(value, OrderedDict):
         raise QAPIExprError(expr_info,
-                            "%s should be a dictionary" % source)
+                            source + " should be a dictionary")
     if not allow_dict:
         raise QAPIExprError(expr_info,
-                            "%s should be a type name" % source)
+                            source + " should be a type name")
     for (key, arg) in value.items():
-        check_name(expr_info, "Member of %s" % source, key,
+        check_name(expr_info, "Member of " + source, key,
                    allow_optional=allow_optional)
         # Todo: allow dictionaries to represent default values of
         # an optional argument.
@@ -433,13 +433,13 @@ def check_command(expr, expr_info):
     name = expr['command']
     allow_star = expr.has_key('gen')

-    check_type(expr_info, "'data' for command '%s'" % name,
+    check_type(expr_info, "'data' for command '" + name + "'",
                expr.get('data'), allow_dict=True, allow_optional=True,
                allow_metas=['union', 'struct'], allow_star=allow_star)
     returns_meta = ['union', 'struct']
     if name in returns_whitelist:
         returns_meta += ['built-in', 'alternate', 'enum']
-    check_type(expr_info, "'returns' for command '%s'" % name,
+    check_type(expr_info, "'returns' for command '" + name + "'",
                expr.get('returns'), allow_array=True, allow_dict=True,
                allow_optional=True, allow_metas=returns_meta,
                allow_star=allow_star)
@@ -452,7 +452,7 @@ def check_event(expr, expr_info):
     if name.upper() == 'MAX':
         raise QAPIExprError(expr_info, "Event name 'MAX' cannot be created")
     events.append(name)
-    check_type(expr_info, "'data' for event '%s'" % name,
+    check_type(expr_info, "'data' for event '" + name + "'",
                expr.get('data'), allow_dict=True, allow_optional=True,
                allow_metas=['union', 'struct'])

@@ -469,7 +469,7 @@ def check_union(expr, expr_info):
         if discriminator is None:
             raise QAPIExprError(expr_info,
                                 "Union '%s' requires a discriminator to go "
-                                "along with base" %name)
+                                "along with base" % name)

     # Two types of unions, determined by discriminator.

@@ -497,7 +497,7 @@ def check_union(expr, expr_info):

         # The value of member 'discriminator' must name a non-optional
         # member of the base struct.
-        check_name(expr_info, "Discriminator of flat union '%s'" % name,
+        check_name(expr_info, "Discriminator of flat union '" + name + "'",
                    discriminator)
         discriminator_type = base_fields.get(discriminator)
         if not discriminator_type:
@@ -515,7 +515,7 @@ def check_union(expr, expr_info):

     # Check every branch
     for (key, value) in members.items():
-        check_name(expr_info, "Member of union '%s'" % name, key)
+        check_name(expr_info, "Member of union '" + name + "'", key)

         # Each value must name a known type; furthermore, in flat unions,
         # branches must be a struct with no overlapping member names
@@ -525,7 +525,7 @@ def check_union(expr, expr_info):
             branch_struct = find_struct(value)
             assert branch_struct
             check_member_clash(expr_info, base, branch_struct['data'],
-                               " of branch '%s'" % key)
+                               " of branch '" + key + "'")

         # If the discriminator names an enum type, then all members
         # of 'data' must also be members of the enum type.
@@ -533,8 +533,8 @@ def check_union(expr, expr_info):
             if not key in enum_define['enum_values']:
                 raise QAPIExprError(expr_info,
                                     "Discriminator value '%s' is not found in "
-                                    "enum '%s'" %
-                                    (key, enum_define["enum_name"]))
+                                    "enum '%s'"
+                                    % (key, enum_define["enum_name"]))

         # Otherwise, check for conflicts in the generated enum
         else:
@@ -585,7 +585,7 @@ def check_enum(expr, expr_info):
         raise QAPIExprError(expr_info,
                             "Enum '%s' requires an array for 'data'" % name)
     for member in members:
-        check_name(expr_info, "Member of enum '%s'" %name, member,
+        check_name(expr_info, "Member of enum '" + name + "'", member,
                    enum_member=True)
         key = camel_to_upper(member)
         if key in values:
@@ -598,9 +598,9 @@ def check_struct(expr, expr_info):
     name = expr['struct']
     members = expr['data']

-    check_type(expr_info, "'data' for struct '%s'" % name, members,
+    check_type(expr_info, "'data' for struct '" + name + "'", members,
                allow_dict=True, allow_optional=True)
-    check_type(expr_info, "'base' for struct '%s'" % name, expr.get('base'),
+    check_type(expr_info, "'base' for struct '" + name + "'", expr.get('base'),
                allow_metas=['struct'])
     if expr.get('base'):
         check_member_clash(expr_info, expr['base'], expr['data'])
@@ -698,10 +698,10 @@ def parse_schema(input_file):
             expr = expr_elem['expr']
             if expr.has_key('union'):
                 if not discriminator_find_enum_define(expr):
-                    add_enum('%sKind' % expr['union'], expr_elem['info'],
+                    add_enum(expr['union'] + 'Kind', expr_elem['info'],
                              implicit=True)
             elif expr.has_key('alternate'):
-                add_enum('%sKind' % expr['alternate'], expr_elem['info'],
+                add_enum(expr['alternate'] + 'Kind', expr_elem['info'],
                          implicit=True)

         # Final pass - validate that exprs make sense
@@ -831,7 +831,7 @@ def type_name(value):

 def add_name(name, info, meta, implicit = False):
     global all_names
-    check_name(info, "'%s'" % meta, name)
+    check_name(info, "'" + meta + "'", name)
     if name in all_names:
         raise QAPIExprError(info,
                             "%s '%s' is already defined"
diff --git a/tests/qapi-schema/include-non-file.err b/tests/qapi-schema/include-non-file.err
index 9658c78..da4e257 100644
--- a/tests/qapi-schema/include-non-file.err
+++ b/tests/qapi-schema/include-non-file.err
@@ -1 +1 @@
-tests/qapi-schema/include-non-file.json:1: Expected a file name (string), got: ['foo', 'bar']
+tests/qapi-schema/include-non-file.json:1: Expected a string for 'include' value
-- 
2.1.0