Re: [Qemu-devel] [PATCH v3 08/14] qapi: Make c_type() consistently convert qapi names

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Continuing the string of cleanups for supporting downstream names
> containing '.', this patch focuses on ensuring c_type() can
> handle a downstream name.  This patch alone does not fix the
> places where generator output should be calling this function
> but was open-coding things instead, but it gets us a step closer.
>
> Note that we generate a List type for our builtins; the code has
> to make sure that ['int'] maps to 'intList' (and not 'q_intList'),
> and that a member with type 'int' still maps to the C type 'int';
> on the other hand, a member with a name of 'int' will still map
> to 'q_int' when going through c_name().  This patch had to teach

has to teach

> type_name() to special-case builtins, since it is used by
> c_list_type() which in turn feeds c_type().
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  scripts/qapi.py | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index b9822c6..a1dfc85 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -769,6 +769,9 @@ def c_enum_const(type_name, const_name):
>
>  c_name_trans = string.maketrans('.-', '__')
>
> +# This function is used for converting the name portion of 'name':'type'
> +# into a valid C name, for use as a struct member or substring of a
> +# function name.

Do we use it only for "the name portion of 'name':'type'"?

I'd prefer a more conventional function comment, like

# Map @name to a valid C identifier.
# If @protect, avoid returning certain ticklish identifiers like C
# keywords by prepending "q_".
# <Explanation of intended use goes here>

>  def c_name(name, protect=True):
>      # ANSI X3J11/88-090, 3.1.1
>      c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
> @@ -800,13 +803,18 @@ def c_name(name, protect=True):
>          return "q_" + name
>      return name.translate(c_name_trans)
>
> +# This function is used for computing the C type of a 'member':['name'] 
> array.

Likewise:

# Map type @name to the C typedef name for the list of this type.

>  def c_list_type(name):
> -    return '%sList' % name
> +    return type_name(name) + 'List'
>
> +# This function is used for converting the type of 'member':'name' into a
> +# substring for use in C pointer types or function names.

Likewise:

# Map type @name to its C typedef name.
# <Explanation of intended use goes here>

Consider rename parameter @name, because it can either be a name string,
or a list containing a name string.  Same for the other functions.
Perhaps in a separate patch for easier review.

>  def type_name(name):
>      if type(name) == list:
>          return c_list_type(name[0])
> -    return name
> +    if name in builtin_types.keys():
> +        return name
> +    return c_name(name)

Together with the change to c_list_type(), this changes type_name() as
follows:

* Name FOO becomes c_name(FOO) instead of FOO, except when FOO is the
  name of a built-in type.  Bug fix when FOO contains '.' or '-' or is
  a ticklish identifier other than a built-in type.

* List of FOO becomes c_name(FOO) + "List" instead of FOOList.  Bug fix
  when FOO contains '.' or '-'.  Not a bug fix when ticklish FOO becomes
  q_FOO, but improves consistency with the element type's C name then.

Correct?

>
>  def add_name(name, info, meta, implicit = False):
>      global all_names
> @@ -864,6 +872,7 @@ def is_enum(name):
>
>  eatspace = '\033EATSPACE.'
>
> +# This function is used for computing the full C type of 'member':'name'.
>  # 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().

Likewise:

# Map type @name to its C type expression.
# If @is_param, const-qualify the string type.
# <Explanation of intended use goes here>
# A special suffix...

> @@ -888,13 +897,13 @@ def c_type(name, is_param=False):
>      elif type(name) == list:
>          return '%s *%s' % (c_list_type(name[0]), eatspace)
>      elif is_enum(name):
> -        return name
> +        return c_name(name)
>      elif name == None or len(name) == 0:
>          return 'void'

Aside: len(name) == 0 is a lame way to test name == "".
Aside^2: I wonder whether we ever pass that.

>      elif name in events:
>          return '%sEvent *%s' % (camel_case(name), eatspace)
>      else:
> -        return '%s *%s' % (name, eatspace)
> +        return '%s *%s' % (c_name(name), eatspace)

I figure the else is for complex types.  If that's correct, we should
perhaps add a comment.

>
>  def is_c_ptr(name):
>      suffix = "*" + eatspace

Together with the change to c_list_type(), this changes c_type() as
follows:

* Enum FOO becomes c_name(FOO) instead of FOO.  Bug fix when FOO
  contains '.' or '-' or is a ticklish identifier.

* Complex type FOO becomes c_name(FOO) + "*" instead of FOO *.  Bug fix
  when FOO contains '.' or '-' or is a ticklish identifier.

* List of FOO becomes c_name(FOO) + "List *" instead of FOOList *.  Bug
  fix when FOO contains '.' or '-'.  Not a bug fix when ticklish FOO
  becomes q_FOO, but improves consistency with the element type's C name
  then.

Correct?