From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60966) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZaO3j-0005wT-8k for qemu-devel@nongnu.org; Fri, 11 Sep 2015 09:08:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZaO3f-0007Lq-Hh for qemu-devel@nongnu.org; Fri, 11 Sep 2015 09:08:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33994) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZaO3f-0007Lg-A6 for qemu-devel@nongnu.org; Fri, 11 Sep 2015 09:08:03 -0400 Date: Fri, 11 Sep 2015 14:07:58 +0100 From: "Daniel P. Berrange" Message-ID: <20150911130758.GE21525@redhat.com> References: <1441973427-8897-1-git-send-email-berrange@redhat.com> <1441973427-8897-2-git-send-email-berrange@redhat.com> <55F2CAF4.7090807@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <55F2CAF4.7090807@redhat.com> Subject: Re: [Qemu-devel] [PATCH PULL 01/11] qapi: allow override of default enum prefix naming Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Peter Maydell , qemu-devel@nongnu.org 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 > > --- > > 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 :|