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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3683 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]