Eric Blake <ebl...@redhat.com> writes:
> On 11/19/2015 09:50 AM, Markus Armbruster wrote:
>> Let's think through this on a higher level.
>>
>> I figure the motivation for this patch is twofold:
>>
>> 1. C identifier clash detection
>>
>
>>
>> 2. Dislike for interfaces that differ only in case
>
> And the related dislike for interfaces that differ only in '-' or '_'.
Yes.
>> Our naming conventions actually make clashes due to folding relatively
>> unlikely. They are:
>>
>> * All names consist of letters, digits, '-' and '_', starting with a
>> letter.
>>
>> * Event names are ALL_CAPS, i.e. they use neither lower case nor '-'.
>>
>> Without lower case, '-' and '.', clash due to folding is impossible.
>
> So if we enforce that convention, we don't have to worry about case
> clash, and I don't think we have any outliers to whitelist.
qmp-introspect.c shows none.
>> * Command names, member names and built-in type names are dashed-lower,
>> i.e. they use neither upper case nor '_'.
>>
>> Without upper case, '_', and '.', clash due to folding is impossible.
>
> If we enforce that convention, we have to whitelist outliers (for
> example, 'netdev_add' as a command name, or a number of types whose
> members still use '_').
Yes.
> The existence of a whitelist will discourage
> future clashes, without any of the hassle of coding up clash checks.
I hope so.
>> * Type names are CamelCase. They do not use '_' or '-'.
>>
>> Can theoretically clash in amusing ways: ONegus, OneGus.
>> Ain'tCamelCaseFun!
>
> Do we have any outliers?
Let's look. Stick
print '%s %s' % (entity.__class__.__name__, name)
into QAPISchema.visit()'s loop.
Pipe output through "sed -n 's/^.*Type \([^:].*\)$/\1/p'" to get the
type names. Grep for '[-_]', -v '[a-z]' and -v '[A-Z]' shows there are
no names with [-_], no names without lower case letters, and the only
names without upper case letters are the 14 built-in types. Both for
qapi-schema.json and qga/qapi-schema.json.
>> We can get rid of the clashes by not case folding type names. See
>> "[PATCH RFC 3/5] qapi: Use common name mangling for enumeration
>> constants". The patch additionally drops folding of enumeration
>> member names, which isn't necessary. Controversial.
>
> There is even the possibility of mixed treatment (enumeration name
> portion as-is, member name portion shouted, as in 'MyTypeVALUE1'). Not
> sure if we'll get a clear answer to the controversy, but also not sure
> it is worth holding up other patches while discussing that.
I'm content to shout the member name. I'd stick a '_' between the
prefix and the member name, though.
>> * Additionally, any name may be prefixed by __RFQDN_. RFQDN consists of
>> letters, digits '-' and '.'.
>>
>> Because unprefixed names start with a letter, and the prefix starts
>> with '__', prefixed names cannot clash with unprefixed names.
>>
>> If we additionally stipulated that event prefixes are upper case, and
>> all others lower case, prefixes couldn't contribute to clashes at all.
>
> It's a bit weird that we'd have __org.qemu_command and __ORG.QEMU_EVENT
> for the same vendor.
Yes.
> On the other hand, FQDN is already
> case-insensitive (qemu.org and QEMU.ORG resolve to the same address).
Yes, and that makes stupilating case now somewhat problematic.
> So there won't be clashes between vendors (as no two vendors can share a
> FQDN that differs in case); beyond that, if a vendor wants to play weird
> case games, that's their (downstream) problem, not ours.
Yes, not our worry.
>> Strict adherence to our naming conventions would eliminate clashes due
>> to folding except for type names. A single extra dictionary mapping
>> c_name(typ.name).lower() to typ would suffice.
>
> Certainly may be simpler than carrying three dictionaries for
> command/event/type.
>
>>
>> What happens to the rest of your queue if we shelve this patch for now?
>
> Not much; in fact, according to the patch version log:
>
> ---
> v12: add in entity case collisions (required sharing two maps),
> improve commit message
> v11: rebase to latest, don't focus so hard on future case-insensitive
> extensions, adjust commit message
> v10: new patch
> ---
>
> I only even added it due to conversations on v9, and intentionally kept
> it separate from 'qapi: Detect collisions in C member names' (we
> absolutely want to report exact-name collisions, whether or not we also
> decide to report case collisions). It should not be a problem to defer
> this patch (or a better version of it that enforces conventions, adds
> whitelist handling, then only worries about type name case collisions)
> to much later, if at all.
Okay, let's shelve the patch for now, keep the queue moving, and revisit
the patch at a more opportune time.
I hope the detour didn't eat too much of your time. We learned a few
things, and perhaps we can still reuse some of the work.
