From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55969) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUuzO-0001qu-Ce for qemu-devel@nongnu.org; Thu, 27 Aug 2015 07:05:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZUuzK-0008BB-Qb for qemu-devel@nongnu.org; Thu, 27 Aug 2015 07:05:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43491) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUuzK-0008B0-JD for qemu-devel@nongnu.org; Thu, 27 Aug 2015 07:04:58 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 37809373F23 for ; Thu, 27 Aug 2015 11:04:58 +0000 (UTC) Date: Thu, 27 Aug 2015 12:04:53 +0100 From: "Daniel P. Berrange" Message-ID: <20150827110453.GP24486@redhat.com> References: <1440601524-30316-1-git-send-email-berrange@redhat.com> <1440601524-30316-2-git-send-email-berrange@redhat.com> <55DDD9CF.3090302@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <55DDD9CF.3090302@redhat.com> Subject: Re: [Qemu-devel] [PATCH v5 1/9] 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: Paolo Bonzini , qemu-devel@nongnu.org, Markus Armbruster , Gerd Hoffmann 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 > > --- > > 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 :|