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

[Eric Blake <eblake@redhat.com>]

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

On 11/05/2015 09:01 AM, Daniel P. Berrange wrote:
> 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).

One of the issues at hand is whether we want to (eventually) teach QMP
to be case-insensitive.  Right now, our c_name() mangling preserves case
(you can have a struct with members 'a' and 'A'), although (hopefully)
no one is relying on it.  But camel_to_upper() is case-insensitive ('a'
and 'A' would result in the same enum constant).

In order to (later) support case-insensitive QMP, we need to decide up
front that we will not allow any qapi member names to collide
case-insensitively (outlaw 'a' and 'A' in the same struct; although the
C code is still case-preserving); and now that this series is adding a
single check_clash() function, it's very easy to do.  In fact, I'll add
that to my series for 2.5 (it's always easier to reserve something now,
especially if no one was using it, and then relax later; than it is to
try and restrict things later but run into counter-cases).

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

But this should already be the case - can you come up with a pair of
valid enum values that won't clash under camel_to_upper() but would
result in in a c_name() value collision?  The converse is not true -
there ARE pairs of c_name() values that are distinct, but which map to
the same mangling with camel_to_upper().  But if we insist that names do
not collide case-insensitively, then that issue goes away - having
ALL_CAP enum constants won't cause any collisions even when those
constants are derived from member names, because member names are
already forbidden from case-insensitive clash.

There is still the question of whether we can get rid of the spurious
collision with 'max', by putting the enum sentinel out of the namespace
generated for other values.  But even with ALL_CAPS, that is possible:

        BLOCK_DEVICE_IO_STATUS_OK = 0,
        BLOCK_DEVICE_IO_STATUS_FAILED = 1,
        BLOCK_DEVICE_IO_STATUS_NOSPACE = 2,
        BLOCK_DEVICE_IO_STATUSMAX = 3,

Or insist that no enum value can start with a lone _ (double __ is okay
for downstream extensions, though):

        BLOCK_DEVICE_IO_STATUS_OK = 0,
        BLOCK_DEVICE_IO_STATUS_FAILED = 1,
        BLOCK_DEVICE_IO_STATUS_NOSPACE = 2,
        BLOCK_DEVICE_IO_STATUS__MAX = 3,


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

Basically, prefix + '_' + c_name(value).to_upper().

Auto-generating prefix via heuristics may still be okay, but what we
want to gain is that the suffix portion is easily predictable if we know
the c_name() mangling of the enum identifer.

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

I also feel like this RFC is bordering on feature rather than bugfix,
making it risky to take in 2.5 softfreeze.  If we decide we want this, I
think it belongs better in 2.6.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]