Re: [Qemu-devel] [PATCH v11 19/28] qapi: Change munging of CamelCase enum values

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 11/10/2015 11:51 PM, Eric Blake wrote:
>> When munging enum values, the fact that we were passing the entire
>> prefix + value through camel_to_upper() meant that enum values
>> spelled with CamelCase could be turned into CAMEL_CASE.  However,
>> this provides a potential collision (both OneTwo and One-Two would
>> munge into ONE_TWO).  By changing the generation of enum constants
>> to always be prefix + '_' + c_name(value).upper(), we can avoid
>> any risk of collisions (if we can also ensure no case collisions,
>> in the next patch) without having to think about what the
>> heuristics in camel_to_upper() will actually do to the value.
>> 
>
>> +++ b/scripts/qapi.py
>> @@ -1439,7 +1439,7 @@ def camel_to_upper(value):
>>  def c_enum_const(type_name, const_name, prefix=None):
>>      if prefix is not None:
>>          type_name = prefix
>> -    return camel_to_upper(type_name + '_' + const_name)
>> +    return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()
>
> Doesn't match the commit message, because it used c_name(,False), while
> c_name(name) is short for c_name(name, True).
>
> What's more, looking at it exposed a bug: c_name('_Thread-local')
> returns '_Thread_local', but this collides with a C11 keyword (and was
> supposed to be munged to q__Thread_local); add '_Thread-local':'int' to
> a struct to see the resulting hilarity:
>
>   CC    tests/test-qmp-output-visitor.o
> In file included from tests/test-qmp-output-visitor.c:17:0:
> tests/test-qapi-types.h:781:13: error: expected identifier or ‘(’ before
> ‘_Thread_local’
>      int64_t _Thread_local;
>              ^

c_name() first protects ticklish identifiers, then mangles.  That's
exactly backwards.

> But it also made me realize that c_name('Q-int') happily returns 'Q_int'
> (we only reserved the leading 'q_' namespace, not 'Q_').  Alas, that
> means c_name('Q-int', False).upper() and c_name('int', False).upper()
> both produce 'Q_INT', and we have a collision.  So I think enum names
> have to be munged by c_name(name, True).

Our goal is to have simple rules for reserved names and collisions, and
resonably simple code to catch them.

"[PATCH 20] qapi: Forbid case-insensitive clashes" is incomplete --- if
we make clashing case-insensitive, we need to reserve names
case-insensitively, too.

We need c_name() to protect ticklish identifiers only when its result is
used as identifier.  Not when it's *part* of an identifier,
e.g. prefixed with qapi_, or camel_to_upper(type_name) + '_'.

We can protect even when we don't need to, if that helps keeping things
simple.

The obvious simple way to check for collisions works like this:

1. Every QAPI name is mangled in exactly one way, modulo case: always
   with c_name(), and always with the same value of protect.

2. We require the mangled name to be case-insensitively unique in its
   name space.

Any holes left?