Re: [Qemu-devel] [PATCH PULL 01/11] qapi: allow override of default enum prefix naming

[Daniel P. Berrange]

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