| Message ID | 1441973427-8897-2-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/11/2015 06:10 AM, Daniel P. Berrange wrote: > The camel_to_upper() method applies some heuristics to turn > a mixed case type name into an all-uppercase name. This is > used for example, to generate enum constant name prefixes. > > The heuristics don't also generate a satisfactory name > though. eg > > { 'enum': 'QCryptoTLSCredsEndpoint', > 'data': ['client', 'server']} > > Results in Q_CRYPTOTLS_CREDS_ENDPOINT_CLIENT. This has > an undesirable _ after the initial Q and is missing an > _ between the CRYPTO & TLS strings. > > Rather than try to add more and more heuristics to try > to cope with this, simply allow the QAPI schema to > specify the desired enum constant prefix explicitly. > > eg > > { 'enum': 'QCryptoTLSCredsEndpoint', > 'prefix': 'QCRYPTO_TLS_CREDS_ENDPOINT', > 'data': ['client', 'server']} > > Now gives the QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT name. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- Missing Reviewed-by tags. Probably because I haven't had a chance to look at v6 yet, and didn't leave R-b on v5. Paolo gave blanket review, but I'm still interested in reviewing. In particular, > +++ b/docs/qapi-code-gen.txt > @@ -236,6 +236,7 @@ both fields like this: > === Enumeration types === > > Usage: { 'enum': STRING, 'data': ARRAY-OF-STRING } > + { 'enum': STRING, 'prefix': STRING, 'data': ARRAY-OF-STRING } s/'prefix'/'*prefix'/ to mark that prefix is optional. > +++ b/scripts/qapi-types.py > @@ -348,9 +348,11 @@ for expr in exprs: > if expr.has_key('struct'): > ret += generate_fwd_struct(expr['struct']) > elif expr.has_key('enum'): > - ret += generate_enum(expr['enum'], expr['data']) > + ret += generate_enum(expr['enum'], expr['data'], > + expr.get('prefix')) > ret += generate_fwd_enum_struct(expr['enum']) > - fdef.write(generate_enum_lookup(expr['enum'], expr['data'])) > + fdef.write(generate_enum_lookup(expr['enum'], expr['data'], > + expr.get('prefix'))) > elif expr.has_key('union'): And this probably introduces merge conflicts with Markus' introspection work. I'm not sure which series should go in first, but Markus' has certainly been on the list longer and has higher complexity. > +++ b/tests/Makefile > @@ -221,7 +221,8 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \ > comments.json empty.json enum-empty.json enum-missing-data.json \ > enum-wrong-data.json enum-int-member.json enum-dict-member.json \ > enum-clash-member.json enum-max-member.json enum-union-clash.json \ > - enum-bad-name.json funny-char.json indented-expr.json \ > + enum-bad-name.json enum-bad-prefix.json \ > + funny-char.json indented-expr.json \ > missing-type.json bad-ident.json ident-with-escape.json \ > escape-outside-string.json unknown-escape.json \ > escape-too-short.json escape-too-big.json unicode-str.json \ > @@ -520,7 +521,7 @@ $(patsubst %, check-%, $(check-qapi-schema-y)): check-%.json: $(SRC_PATH)/%.json > " TEST $*.out") > @diff -q $(SRC_PATH)/$*.out $*.test.out > @# Sanitize error messages (make them independent of build directory) > - @perl -p -e 's|\Q$(SRC_PATH)\E/||g' $*.test.err | diff -q $(SRC_PATH)/$*.err - > + @perl -p -e 's|\Q$(SRC_PATH)\E/||g' $*.test.err | diff $(SRC_PATH)/$*.err - This hunk feels like it might need to be a separate patch, or at least explained in the commit message.
On Fri, Sep 11, 2015 at 06:37:08AM -0600, Eric Blake wrote: > On 09/11/2015 06:10 AM, Daniel P. Berrange wrote: > > The camel_to_upper() method applies some heuristics to turn > > a mixed case type name into an all-uppercase name. This is > > used for example, to generate enum constant name prefixes. > > > > The heuristics don't also generate a satisfactory name > > though. eg > > > > { 'enum': 'QCryptoTLSCredsEndpoint', > > 'data': ['client', 'server']} > > > > Results in Q_CRYPTOTLS_CREDS_ENDPOINT_CLIENT. This has > > an undesirable _ after the initial Q and is missing an > > _ between the CRYPTO & TLS strings. > > > > Rather than try to add more and more heuristics to try > > to cope with this, simply allow the QAPI schema to > > specify the desired enum constant prefix explicitly. > > > > eg > > > > { 'enum': 'QCryptoTLSCredsEndpoint', > > 'prefix': 'QCRYPTO_TLS_CREDS_ENDPOINT', > > 'data': ['client', 'server']} > > > > Now gives the QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT name. > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > Missing Reviewed-by tags. Probably because I haven't had a chance to > look at v6 yet, and didn't leave R-b on v5. Paolo gave blanket review, > but I'm still interested in reviewing. In particular, NB, I removed Reviewed-by tags from any patch which I had to change since a previous posting anyway, and there was a conflict with master that I had to resolve on this one. > > > > +++ b/docs/qapi-code-gen.txt > > @@ -236,6 +236,7 @@ both fields like this: > > === Enumeration types === > > > > Usage: { 'enum': STRING, 'data': ARRAY-OF-STRING } > > + { 'enum': STRING, 'prefix': STRING, 'data': ARRAY-OF-STRING } > > s/'prefix'/'*prefix'/ > > to mark that prefix is optional. OK > > +++ b/scripts/qapi-types.py > > > @@ -348,9 +348,11 @@ for expr in exprs: > > if expr.has_key('struct'): > > ret += generate_fwd_struct(expr['struct']) > > elif expr.has_key('enum'): > > - ret += generate_enum(expr['enum'], expr['data']) > > + ret += generate_enum(expr['enum'], expr['data'], > > + expr.get('prefix')) > > ret += generate_fwd_enum_struct(expr['enum']) > > - fdef.write(generate_enum_lookup(expr['enum'], expr['data'])) > > + fdef.write(generate_enum_lookup(expr['enum'], expr['data'], > > + expr.get('prefix'))) > > elif expr.has_key('union'): > > And this probably introduces merge conflicts with Markus' introspection > work. I'm not sure which series should go in first, but Markus' has > certainly been on the list longer and has higher complexity. I've been looking Markus' work but it is still an RFC, so unless a pull request is going to be sent for it in the next week, I would rather not delay this series merging, as there is further work depending on it. > > @@ -520,7 +521,7 @@ $(patsubst %, check-%, $(check-qapi-schema-y)): check-%.json: $(SRC_PATH)/%.json > > " TEST $*.out") > > @diff -q $(SRC_PATH)/$*.out $*.test.out > > @# Sanitize error messages (make them independent of build directory) > > - @perl -p -e 's|\Q$(SRC_PATH)\E/||g' $*.test.err | diff -q $(SRC_PATH)/$*.err - > > + @perl -p -e 's|\Q$(SRC_PATH)\E/||g' $*.test.err | diff $(SRC_PATH)/$*.err - > > This hunk feels like it might need to be a separate patch, or at least > explained in the commit message. Yeha, guess I could put it in a separate patch - it just lets the developer see the actual error instead of a generic useless message about failure. Regards, Daniel
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index ff16df2..917409c 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -236,6 +236,7 @@ both fields like this: === Enumeration types === Usage: { 'enum': STRING, 'data': ARRAY-OF-STRING } + { 'enum': STRING, 'prefix': STRING, 'data': ARRAY-OF-STRING } An enumeration type is a dictionary containing a single 'data' key whose value is a list of strings. An example enumeration is: @@ -247,6 +248,13 @@ useful. The list of strings should be lower case; if an enum name represents multiple words, use '-' between words. The string 'max' is not allowed as an enum value, and values should not be repeated. +The enum constants will be named by using a heuristic to turn the +type name into a set of underscore separated words. For the example +above, 'MyEnum' will turn into 'MY_ENUM' giving a constant name +of 'MY_ENUM_VALUE1' for the first value. If the default heuristic +does not result in a desirable name, the optional 'prefix' field +can be used when defining the enum. + The enumeration values are passed as strings over the Client JSON Protocol, but are encoded as C enum integral values in generated code. While the C code starts numbering at 0, it is better to use explicit diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index f2428f3..a8453d1 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -101,20 +101,20 @@ struct %(name)s { return ret -def generate_enum_lookup(name, values): +def generate_enum_lookup(name, values, prefix=None): ret = mcgen(''' const char *const %(name)s_lookup[] = { ''', name=c_name(name)) for value in values: - index = c_enum_const(name, value) + index = c_enum_const(name, value, prefix) ret += mcgen(''' [%(index)s] = "%(value)s", ''', index = index, value = value) - max_index = c_enum_const(name, 'MAX') + max_index = c_enum_const(name, 'MAX', prefix) ret += mcgen(''' [%(max_index)s] = NULL, }; @@ -122,7 +122,7 @@ const char *const %(name)s_lookup[] = { max_index=max_index) return ret -def generate_enum(name, values): +def generate_enum(name, values, prefix=None): name = c_name(name) lookup_decl = mcgen(''' @@ -141,7 +141,7 @@ typedef enum %(name)s { i = 0 for value in enum_values: - enum_full_value = c_enum_const(name, value) + enum_full_value = c_enum_const(name, value, prefix) enum_decl += mcgen(''' %(enum_full_value)s = %(i)d, ''', @@ -348,9 +348,11 @@ for expr in exprs: if expr.has_key('struct'): ret += generate_fwd_struct(expr['struct']) elif expr.has_key('enum'): - ret += generate_enum(expr['enum'], expr['data']) + ret += generate_enum(expr['enum'], expr['data'], + expr.get('prefix')) ret += generate_fwd_enum_struct(expr['enum']) - fdef.write(generate_enum_lookup(expr['enum'], expr['data'])) + fdef.write(generate_enum_lookup(expr['enum'], expr['data'], + expr.get('prefix'))) elif expr.has_key('union'): ret += generate_fwd_struct(expr['union']) enum_define = discriminator_find_enum_define(expr) diff --git a/scripts/qapi.py b/scripts/qapi.py index 817d824..c8521d1 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -634,11 +634,15 @@ def check_alternate(expr, expr_info): def check_enum(expr, expr_info): name = expr['enum'] members = expr.get('data') + prefix = expr.get('prefix') values = { 'MAX': '(automatic)' } if not isinstance(members, list): raise QAPIExprError(expr_info, "Enum '%s' requires an array for 'data'" % name) + if prefix is not None and not isinstance(prefix, str): + raise QAPIExprError(expr_info, + "Enum '%s' requires a string for 'prefix'" % name) for member in members: check_name(expr_info, "Member of enum '%s'" %name, member, enum_member=True) @@ -693,7 +697,7 @@ def check_exprs(exprs): expr = expr_elem['expr'] info = expr_elem['info'] if expr.has_key('enum'): - check_keys(expr_elem, 'enum', ['data']) + check_keys(expr_elem, 'enum', ['data'], ['prefix']) add_enum(expr['enum'], info, expr['data']) elif expr.has_key('union'): check_keys(expr_elem, 'union', ['data'], @@ -813,7 +817,9 @@ def camel_to_upper(value): new_name += c return new_name.lstrip('_').upper() -def c_enum_const(type_name, const_name): +def c_enum_const(type_name, const_name, prefix=None): + if prefix is not None: + type_name = prefix return camel_to_upper(type_name + '_' + const_name) c_name_trans = string.maketrans('.-', '__') diff --git a/tests/Makefile b/tests/Makefile index 34c6136..01ae558 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -221,7 +221,8 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \ comments.json empty.json enum-empty.json enum-missing-data.json \ enum-wrong-data.json enum-int-member.json enum-dict-member.json \ enum-clash-member.json enum-max-member.json enum-union-clash.json \ - enum-bad-name.json funny-char.json indented-expr.json \ + enum-bad-name.json enum-bad-prefix.json \ + funny-char.json indented-expr.json \ missing-type.json bad-ident.json ident-with-escape.json \ escape-outside-string.json unknown-escape.json \ escape-too-short.json escape-too-big.json unicode-str.json \ @@ -520,7 +521,7 @@ $(patsubst %, check-%, $(check-qapi-schema-y)): check-%.json: $(SRC_PATH)/%.json " TEST $*.out") @diff -q $(SRC_PATH)/$*.out $*.test.out @# Sanitize error messages (make them independent of build directory) - @perl -p -e 's|\Q$(SRC_PATH)\E/||g' $*.test.err | diff -q $(SRC_PATH)/$*.err - + @perl -p -e 's|\Q$(SRC_PATH)\E/||g' $*.test.err | diff $(SRC_PATH)/$*.err - @diff -q $(SRC_PATH)/$*.exit $*.test.exit # Consolidated targets diff --git a/tests/qapi-schema/enum-bad-prefix.err b/tests/qapi-schema/enum-bad-prefix.err new file mode 100644 index 0000000..399f5f7 --- /dev/null +++ b/tests/qapi-schema/enum-bad-prefix.err @@ -0,0 +1 @@ +tests/qapi-schema/enum-bad-prefix.json:2: Enum 'MyEnum' requires a string for 'prefix' diff --git a/tests/qapi-schema/enum-bad-prefix.exit b/tests/qapi-schema/enum-bad-prefix.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/enum-bad-prefix.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/enum-bad-prefix.json b/tests/qapi-schema/enum-bad-prefix.json new file mode 100644 index 0000000..996f628 --- /dev/null +++ b/tests/qapi-schema/enum-bad-prefix.json @@ -0,0 +1,2 @@ +# The prefix must be a string type +{ 'enum': 'MyEnum', 'data': [ 'one' ], 'prefix': [ 'fish' ] } diff --git a/tests/qapi-schema/enum-bad-prefix.out b/tests/qapi-schema/enum-bad-prefix.out new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index a9e5aab..677190c 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -6,6 +6,11 @@ { 'struct': 'NestedEnumsOne', 'data': { 'enum1': 'EnumOne', '*enum2': 'EnumOne', 'enum3': 'EnumOne', '*enum4': 'EnumOne' } } +# for testing override of default naming heuristic +{ 'enum': 'QEnumTwo', + 'prefix': 'QENUM_TWO', + 'data': [ 'value1', 'value2' ] } + # for testing nested structs { 'struct': 'UserDefOne', 'base': 'UserDefZero', # intentional forward reference diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index b0b7187..32f17c5 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -1,5 +1,6 @@ [OrderedDict([('enum', 'EnumOne'), ('data', ['value1', 'value2', 'value3'])]), OrderedDict([('struct', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]), + OrderedDict([('enum', 'QEnumTwo'), ('prefix', 'QENUM_TWO'), ('data', ['value1', 'value2'])]), OrderedDict([('struct', 'UserDefOne'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('*enum1', 'EnumOne')]))]), OrderedDict([('struct', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]), OrderedDict([('struct', 'UserDefTwoDictDict'), ('data', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]), @@ -33,6 +34,7 @@ OrderedDict([('event', '__ORG.QEMU_X-EVENT'), ('data', '__org.qemu_x-Struct')]), OrderedDict([('command', '__org.qemu_x-command'), ('data', OrderedDict([('a', ['__org.qemu_x-Enum']), ('b', ['__org.qemu_x-Struct']), ('c', '__org.qemu_x-Union2'), ('d', '__org.qemu_x-Alt')])), ('returns', '__org.qemu_x-Union1')])] [{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']}, + {'enum_name': 'QEnumTwo', 'enum_values': ['value1', 'value2']}, {'enum_name': '__org.qemu_x-Enum', 'enum_values': ['__org.qemu_x-value']}, {'enum_name': 'UserDefAlternateKind', 'enum_values': None}, {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None},
The camel_to_upper() method applies some heuristics to turn a mixed case type name into an all-uppercase name. This is used for example, to generate enum constant name prefixes. The heuristics don't also generate a satisfactory name though. eg { 'enum': 'QCryptoTLSCredsEndpoint', 'data': ['client', 'server']} Results in Q_CRYPTOTLS_CREDS_ENDPOINT_CLIENT. This has an undesirable _ after the initial Q and is missing an _ between the CRYPTO & TLS strings. Rather than try to add more and more heuristics to try to cope with this, simply allow the QAPI schema to specify the desired enum constant prefix explicitly. eg { 'enum': 'QCryptoTLSCredsEndpoint', 'prefix': 'QCRYPTO_TLS_CREDS_ENDPOINT', 'data': ['client', 'server']} Now gives the QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT name. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- docs/qapi-code-gen.txt | 8 ++++++++ scripts/qapi-types.py | 16 +++++++++------- scripts/qapi.py | 10 ++++++++-- tests/Makefile | 5 +++-- tests/qapi-schema/enum-bad-prefix.err | 1 + tests/qapi-schema/enum-bad-prefix.exit | 1 + tests/qapi-schema/enum-bad-prefix.json | 2 ++ tests/qapi-schema/enum-bad-prefix.out | 0 tests/qapi-schema/qapi-schema-test.json | 5 +++++ tests/qapi-schema/qapi-schema-test.out | 2 ++ 10 files changed, 39 insertions(+), 11 deletions(-) create mode 100644 tests/qapi-schema/enum-bad-prefix.err create mode 100644 tests/qapi-schema/enum-bad-prefix.exit create mode 100644 tests/qapi-schema/enum-bad-prefix.json create mode 100644 tests/qapi-schema/enum-bad-prefix.out