Re: [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants

["Daniel P. Berrange" <berrange@redhat.com>]

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

On Mon, Nov 09, 2015 at 10:34:41AM +0100, Markus Armbruster wrote:
> "Daniel P. Berrange" <berrange@redhat.com> 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 :|

[-- Attachment #2: check-enum.pl --]
[-- Type: application/x-perl, Size: 1384 bytes --]