[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enum
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants |
Date: |
Mon, 9 Nov 2015 10:53:18 +0000 |
User-agent: |
Mutt/1.5.23 (2015-06-09) |
On Mon, Nov 09, 2015 at 10:34:41AM +0100, Markus Armbruster wrote:
> "Daniel P. Berrange" <address@hidden> writes:
>
> > On Thu, Nov 05, 2015 at 04:30:00PM +0100, Markus Armbruster wrote:
> >> QAPI names needn't be valid C identifiers, so we mangle them with
> >> c_name(). Except for enumeration constants, which we mangle with
> >> camel_to_upper().
> >>
> >> c_name() is easy enough to understand: replace '.' and '-' by '_',
> >> prefix certain ticklish identifiers with 'q_'.
> >>
> >> camel_to_upper() is a hairball of heuristics, and guessing how it'll
> >> mangle interesting input could serve as a (nerdy) game. Despite some
> >> tweaking (commit 5d371f4), it's still inadqeuate for some QAPI names
> >> (commit 351d36e).
> >>
> >> Example: QAPI definition
> >>
> >> { 'enum': 'BlockDeviceIoStatus', 'data': [ 'ok', 'failed', 'nospace' ]
> >> }
> >>
> >> generates
> >>
> >> typedef enum BlockDeviceIoStatus {
> >> BLOCK_DEVICE_IO_STATUS_OK = 0,
> >> BLOCK_DEVICE_IO_STATUS_FAILED = 1,
> >> BLOCK_DEVICE_IO_STATUS_NOSPACE = 2,
> >> BLOCK_DEVICE_IO_STATUS_MAX = 3,
> >> } BlockDeviceIoStatus;
> >>
> >> Observe that c_name() maps BlockDeviceIoStatus to itself, and
> >> camel_to_upper() maps it to BLOCK_DEVICE_IO_STATUS, i.e. the
> >> enumeration constants are shouted, the enumeration type isn't.
> >>
> >> Because mangled names must not clash, name mangling restricts the
> >> names you can use. For example, you can't have member 'a-b' and
> >> 'a_b'.
> >>
> >> Having two separate manglings complicates this. Enumeration constants
> >> must be distinct after mangling with camel_to_upper(). But as soon as
> >> you use the enumeration as union tag, they must *also* be distinct
> >> after mangling with c_name().
> >>
> >> Having shouted enumeration constants isn't worth the complexity cost.
> >
> > I rather disagree with this. Having all-uppercase for enum constants
> > is a really widely used convention and I think it is pretty useful
> > when reading to code to have constants (whether #define/enum) clearly
> > marked in uppercase. After this change we'll have situation where
> > QAPI enums uses CamelCase, while all other non-QAPI enums (whether
> > defined in QEMU source, or via #included 3rd party headers use
> > UPPER_CASE for constants. I think that's rather unpleasant.
>
> CODING_STYLE doesn't mandate shouting constants.
>
> Existing code doesn't shout constants consistently.
I wrote a script (attached) to parse the code and try to get extract
some data on upper/lower/mixed case usage in enum declarations
Enum member naming
Uppercase: 993 in 325 files
Lowercase: 119 in 76 files
Mixedcase: 110 in 22 files
So, uppercase is preferred 10:1 over both lowercase and mixedcase
The filecount for uppercase would be far higher were it not for
fact that all QAPI enums end up in one file.
If we try to count #define's which provide constants, again
all uppercase is the clear majority
Upper 32256
Lower 829
Mixed 8869
(50% of those Mixed constants come from the userspace emulator
#defines for syscall numbers)
> Third parties don't shout constants consistently.
Running the same script across all the QEMU RPM dependancies again
shows uppercase is the favoured style:
Enum member naming
Uppercase: 538 in 178 files
Lowercase: 38 in 17 files
Mixedcase: 141 in 53 files
The breakdown per-RPM is as follows:
bluez-libs-devel
Enum member naming
Uppercase: 4 in 3 files
brlapi-devel
Enum member naming
Uppercase: 43 in 23 files
Mixedcase: 83 in 29 files
bzip2-devel
Enum member naming
None
ceph-devel
Enum member naming
None
cyrus-sasl-devel
Enum member naming
Uppercase: 1 in 1 files
glusterfs-api-devel
Enum member naming
Uppercase: 1 in 1 files
glusterfs-devel
Enum member naming
Uppercase: 94 in 26 files
Lowercase: 10 in 6 files
Mixedcase: 2 in 1 files
gnutls-devel
Enum member naming
Uppercase: 63 in 10 files
gtk3-devel
Enum member naming
Uppercase: 152 in 60 files
Lowercase: 1 in 1 files
libaio-devel
Enum member naming
Uppercase: 1 in 1 files
libattr-devel
Enum member naming
None
libcap-devel
Enum member naming
None
libcurl-devel
Enum member naming
Uppercase: 17 in 2 files
Lowercase: 3 in 1 files
Mixedcase: 1 in 1 files
libfdt-devel
Enum member naming
None
libiscsi-devel
Enum member naming
Uppercase: 32 in 2 files
libjpeg-devel
Enum member naming
None
libpng-devel
Enum member naming
None
librdmacm-devel
Enum member naming
Uppercase: 7 in 3 files
libseccomp-devel
Enum member naming
Uppercase: 2 in 1 files
libssh2-devel
Enum member naming
None
libusbx-devel
Enum member naming
Uppercase: 21 in 1 files
libuuid-devel
Enum member naming
None
ncurses-devel
Enum member naming
Uppercase: 3 in 3 files
Mixedcase: 6 in 6 files
nss-devel
Enum member naming
Uppercase: 2 in 2 files
Lowercase: 10 in 2 files
Mixedcase: 41 in 10 files
numactl-devel
Enum member naming
None
pciutils-devel
Enum member naming
Uppercase: 2 in 1 files
pixman-devel
Enum member naming
Uppercase: 5 in 1 files
Mixedcase: 1 in 1 files
pulseaudio-libs-devel
Enum member naming
Uppercase: 17 in 6 files
SDL2-devel
Enum member naming
Uppercase: 30 in 17 files
Mixedcase: 3 in 2 files
spice-server-devel
Enum member naming
Uppercase: 5 in 3 files
systemtap-sdt-devel
Enum member naming
None
usbredir-devel
Enum member naming
Lowercase: 11 in 4 files
vte3-devel
Enum member naming
Uppercase: 7 in 3 files
xen-devel
Enum member naming
Uppercase: 29 in 8 files
Lowercase: 3 in 3 files
Mixedcase: 2 in 2 files
xfsprogs-devel
Enum member naming
Mixedcase: 2 in 1 files
zlib-devel
Enum member naming
None
> A competing convention is to use the enumeration type's name as prefix
> for the constants.
That is counted by the 'mixedcase' stats and as seen above that's still
10:1 less popular than all uppercase.
>
> > I agree with your premise that predicting the output of the qapi
> > name mangler though is essentially impossible for mortals. How
> > about a counter proposal....
> >
> > First make 'prefix' compulsory for enums, instead of trying to
> > figure out a suitable prefix automatically. Then, have a very
> > simple rule for the enum constants where you just uppercase a-z
> > and translate any non-alpha-numeric character into a _. Don't
> > try todo anything else special like figuring out word boundaries.
>
> Essentially c_name(qapi_name).upper().
>
> > That would get of much of the complexity in the qapi name mangler
> > and give a easily predictable enum constant name. Thus the vast
> > majority of .c source files (possibly even all of them) would not
> > need any change.
>
> 'prefix' is a work-around for deficient name mangling. Making it
> mandatory declares defeat on enumeration constant name mangling. The
> reason for defeat are unreasonable goals, namely combining these three
> conventions:
>
> * QAPI and C type names are in CamelCase
>
> * Enumeration constants are prefixed by the type name
>
> * Enumeration constants are shouted, including the prefix
>
> They necessitate converting the CamelCase prefix to shouting, which is
> the troublesome part. Note that shouting the remainder (the QAPI member
> name) is trivial: .upper() serves.
>
>
> Let's take a step back and examine what I want to achieve.
>
>
> First, I want simple, consistent rules for QAPI names. Two kinds:
> reserved names and name clashes.
>
> A few names are globally reserved (e.g. prefix 'q_') , and a few more
> only in certain name spaces (e.g. type name suffix 'List'). Simple
> enough.
>
> Two names clash if they're equal after replacing '-' and '.' by '_'.
> Simple enough.
>
> *Except* for enumeration member names, which can clash in other ways
> (example: 'GotCha' with 'got-cha'). The exact special clashing rules
> haven't been written down. Nobody knows them, actually. Instead of
> writing them down, I want to get rid of then.
>
> We could change the normal clashing rule to additionally ignore case.
> Still simple enough, and makes sense to me.
>
> Ignoring case would let us safely shout names in generated code.
>
>
> Second, I want our C names generated in a simple, predictable way. This
> is largely the case:
>
> * We use the QAPI name with '-' and '.' replaced by '_'
>
> * Sometimes, we prepend a 'q_' to avoid certain ticklish names
>
> * We prepend and append fixed strings
>
> *Except* for enumeration member names, which undergo a different
> mangling that is neither simple nor predictable.
>
> My proposal simply drops the special case.
This is really all about the POV you look at the problem from. From
the code generator's POV having enum's all uppercase is a special
case scenario. From the general developer's POV, having enums all
uppercase is the normal case scenario. IMHO the code generator is
there to serve the developers, so following the developer's normal
case should be the priority, even if that makes life harder for
maintaining the code generator.
> Your counter-proposal instead moves the name mangling from the generator
> to the QAPI schema. In other words, it abuses the programmer as name
> mangler. I don't like that, and I wouldn't call it "simple". It does
> address the "not predictable" part.
Saying it abuses the programmer is pretty extreme. When a programmer
is defining an enum, they will always intuitively have an idea of
what they expect the enum constants to look like. Simply defining
that in the QAPI schema at that time is no burden at all and really
is simple/trivial for the programmer.
> If we must have shouting in enumeration constants, we could do the
> following: use the unadulterated type name as prefix, shout the member
> name. Example:
>
> typedef enum BlockDeviceIoStatus {
> BlockDeviceIoStatus_OK = 0,
> BlockDeviceIoStatus_FAILED = 1,
> BlockDeviceIoStatus_NOSPACE = 2,
> BlockDeviceIoStatus_MAX = 3,
> } BlockDeviceIoStatus;
>
> If the QAPI enumeration constant rename flag day bothers us, we can keep
> the 'prefix' feature for now, use it to avoid the renames that touch
> code we don't want to touch now, then rename them one by one at our
> convenience.
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 :|
check-enum.pl
Description: Perl program
- [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants, (continued)
- [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants, Markus Armbruster, 2015/11/05
- Re: [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants, Daniel P. Berrange, 2015/11/05
- Re: [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants, Eric Blake, 2015/11/05
- Re: [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants, Eric Blake, 2015/11/05
- Re: [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants, Markus Armbruster, 2015/11/06
- Re: [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants, Markus Armbruster, 2015/11/06
- [Qemu-devel] What to do about QAPI naming convention violations (was: [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants), Markus Armbruster, 2015/11/10
- [Qemu-devel] blkdebug event names [was: What to do about QAPI naming convention violations], Eric Blake, 2015/11/16
- Re: [Qemu-devel] blkdebug event names [was: What to do about QAPI naming convention violations], Markus Armbruster, 2015/11/17
- Re: [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants, Markus Armbruster, 2015/11/09
- Re: [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants,
Daniel P. Berrange <=
[Qemu-devel] [PATCH v9 17/27] qapi: Clean up after previous commit, Eric Blake, 2015/11/04
[Qemu-devel] [PATCH v9 27/27] qapi: Simplify visits of optional fields, Eric Blake, 2015/11/04
[Qemu-devel] [PATCH v9 23/27] qapi: Simplify visiting of alternate types, Eric Blake, 2015/11/04
Re: [Qemu-devel] [PATCH v9 00/27] alternate layout (post-introspection cleanups, subset C), Markus Armbruster, 2015/11/04
Re: [Qemu-devel] [PATCH v9 00/27] alternate layout (post-introspection cleanups, subset C), Markus Armbruster, 2015/11/05