| Message ID | 1440601524-30316-2-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/26/2015 09:05 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 > _ betweeen the CRYPTO & TLS strings. s/betweeen/between/ > > 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. Idea seems reasonable. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > scripts/qapi-types.py | 14 +++++++------- > scripts/qapi.py | 9 ++++++--- > 2 files changed, 13 insertions(+), 10 deletions(-) Missing documentation (docs/qapi-code-gen.txt) and a testsuite addition. I suggest using 'prefix' on one of the existing enums in tests/qapi-schema/qapi-schema-test.json, then fixing any fallout from 'make check-unit check-qapi-schema' to ensure it still passes - probably done correctly if this also touches tests/qapi-schema/qapi-schema-test.out and tests/test-qmp-*visitor.c. Doesn't affect QMP wire ABI, so the new key shouldn't impact the output produced by Markus' pending series on introspection, although we may have some code collisions with the refactoring work going on there. > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index e6eb4b6..549c2f7 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -336,9 +336,9 @@ 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']) + "\n" > + ret += generate_enum(expr['enum'], expr['data'], expr.get('prefix')) + "\n" > 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'))) Long line; I'd wrap after the last ',' > +++ b/scripts/qapi.py > @@ -698,7 +698,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']) I'd also amend check_enum() to ensure that the supplied prefix is a string (and not some other data structure); if you add a new error message that explicitly filters out an invalid prefix, then that is a further testsuite addition of a new negative test (tests/Makefile.am to add the the new tests/qapi-schema/*.json file, plus the corresponding .{out,exit,err} files to match expected results). > -def c_enum_const(type_name, const_name): > - return camel_to_upper(type_name + '_' + const_name) > +def c_enum_const(type_name, const_name, prefix=None): > + if prefix is not None: > + return prefix + '_' + camel_to_upper(const_name) > + else: > + return camel_to_upper(type_name + '_' + const_name) Would it be any easier to read as: def c_enum_const(type_name, const_name, prefix=None): if not prefix: prefix = camel_to_upper(type_name) return prefix + '_' + camel_to_upper(const_name) But I'm not sure if that would introduce any subtle changes to existing enums.
On Wed, Aug 26, 2015 at 09:22:55AM -0600, Eric Blake wrote: > On 08/26/2015 09:05 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 > > _ betweeen the CRYPTO & TLS strings. > > s/betweeen/between/ > > > > > 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. > > Idea seems reasonable. > > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > scripts/qapi-types.py | 14 +++++++------- > > scripts/qapi.py | 9 ++++++--- > > 2 files changed, 13 insertions(+), 10 deletions(-) > > Missing documentation (docs/qapi-code-gen.txt) and a testsuite addition. > I suggest using 'prefix' on one of the existing enums in > tests/qapi-schema/qapi-schema-test.json, then fixing any fallout from > 'make check-unit check-qapi-schema' to ensure it still passes - probably > done correctly if this also touches > tests/qapi-schema/qapi-schema-test.out and tests/test-qmp-*visitor.c. test-qmp-*visitor is not affected because we're not changing the data types / structure in any way - just the naming of constants and that is not checked by any test-qmp-*vistor test. I've updated qapi-schema-test.out though, and added a test for a bad prefix type. > > +++ b/scripts/qapi.py > > @@ -698,7 +698,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']) > > I'd also amend check_enum() to ensure that the supplied prefix is a > string (and not some other data structure); if you add a new error > message that explicitly filters out an invalid prefix, then that is a > further testsuite addition of a new negative test (tests/Makefile.am to > add the the new tests/qapi-schema/*.json file, plus the corresponding > .{out,exit,err} files to match expected results). Yep, done that now. > > -def c_enum_const(type_name, const_name): > > - return camel_to_upper(type_name + '_' + const_name) > > +def c_enum_const(type_name, const_name, prefix=None): > > + if prefix is not None: > > + return prefix + '_' + camel_to_upper(const_name) > > + else: > > + return camel_to_upper(type_name + '_' + const_name) > > Would it be any easier to read as: > > def c_enum_const(type_name, const_name, prefix=None): > if not prefix: > prefix = camel_to_upper(type_name) > return prefix + '_' + camel_to_upper(const_name) > > But I'm not sure if that would introduce any subtle changes to existing > enums. That doesn't quite work because if the const_name starts with an '_', camel_to_upper would previously collapse the repeated '_'. I can tweak it a bit to be more readable and avoid this problem though. Regards, Daniel
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index e6eb4b6..549c2f7 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -103,20 +103,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)) i = 0 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, }; @@ -125,7 +125,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(''' extern const char * const %(name)s_lookup[]; @@ -143,7 +143,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, ''', @@ -336,9 +336,9 @@ 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']) + "\n" + ret += generate_enum(expr['enum'], expr['data'], expr.get('prefix')) + "\n" 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']) + "\n" enum_define = discriminator_find_enum_define(expr) diff --git a/scripts/qapi.py b/scripts/qapi.py index 06d7fc2..9c1ef36 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -698,7 +698,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'], @@ -818,8 +818,11 @@ def camel_to_upper(value): new_name += c return new_name.lstrip('_').upper() -def c_enum_const(type_name, const_name): - return camel_to_upper(type_name + '_' + const_name) +def c_enum_const(type_name, const_name, prefix=None): + if prefix is not None: + return prefix + '_' + camel_to_upper(const_name) + else: + return camel_to_upper(type_name + '_' + const_name) c_name_trans = string.maketrans('.-', '__')
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 _ betweeen 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> --- scripts/qapi-types.py | 14 +++++++------- scripts/qapi.py | 9 ++++++--- 2 files changed, 13 insertions(+), 10 deletions(-)