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

[Markus Armbruster]

"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.

Third parties don't shout constants consistently.

A competing convention is to use the enumeration type's name as prefix
for the constants.

> 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.

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.


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.