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 <berra...@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
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
