Re: [Qemu-devel] [PATCH v5 1/9] qapi: allow override of default enum prefix naming

[Daniel P. Berrange]

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 <address@hidden>
> > ---
> >  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 :|