Eric Blake <ebl...@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?
