| Message ID | 1430829055-4739-9-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> > --- > 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?
On 05/07/2015 01:39 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> 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(). >> >> >> +# 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. Sure, will split out a second patch, and post v4 shortly. c_name() only operates on strings, but type_name() and c_type() do indeed operate on more than just strings. > >> 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? Yes. 'unix'->"q_unix" but ['unix']->"unixList" was inconsistent, so it is now "q_unixList". But ['int'] must map to "intList" and not "q_intList", because we have implementations for intList but do not need generated code for the builtin type int. > > # 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. I'll find out :) > >> 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. Yes, at this point, all that is left is complex types. > >> >> 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? Yes. I can try and fold that into the commit message.
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. 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. 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. 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) 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(). @@ -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' elif name in events: return '%sEvent *%s' % (camel_case(name), eatspace) else: - return '%s *%s' % (name, eatspace) + return '%s *%s' % (c_name(name), eatspace) def is_c_ptr(name): suffix = "*" + eatspace
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 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 <eblake@redhat.com> --- scripts/qapi.py | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)