[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v12 27/36] qapi: Forbid case-insensitive clashes
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v12 27/36] qapi: Forbid case-insensitive clashes |
Date: |
Wed, 18 Nov 2015 15:09:43 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 11/18/2015 10:08 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
>
>> In general, designing user interfaces that rely on case
>> distinction is poor practice. Another benefit of enforcing
>> a restriction against case-insensitive clashes is that we
>> no longer have to worry about the situation of enum values
>> that could be distinguished by case if mapped by c_name(),
>> but which cannot be distinguished when mapped to C as
>> ALL_CAPS by c_enum_const() [via c_name(name, False).upper()].
>> Thus, having the generator look for case collisions up front
>> will prevent developers from worrying whether different
>> munging rules for member names compared to enum values as a
>> discriminator will cause any problems in qapi unions.
>>
>> @@ -378,9 +379,9 @@ def check_name(expr_info, source, name,
>> allow_optional=False,
>> # code always prefixes it with the enum name
>> if enum_member and membername[0].isdigit():
>> membername = 'D' + membername
>> - # Reserve the entire 'q_' namespace for c_name()
>> + # Reserve the entire 'q_'/'Q_' namespace for c_name()
>> if not valid_name.match(membername) or \
>> - c_name(membername, False).startswith('q_'):
>> + c_name(membername, False).upper().startswith('Q_'):
>> raise QAPIExprError(expr_info,
>> "%s uses invalid name '%s'" % (source, name))
I'll switch to lower() here, for consistency.
> Now let me try claw back some clarity.
>
> First try: refine your approach.
>
> Observe that all but one caller of lookup_entity() actually look up
> types. The one caller that doesn't is _def_entity(). Change it to:
>
> cname = c_name(ent.name)
> if isinstance(ent, QAPISchemaEvent):
> cname = cname.upper()
> else:
> cname = cname.lower()
> if self._lookup_entity(cname)
I think lookup_entity would need two parameters: the munged name, and
the actual name...
> raise QAPIExprError(ent.info,
> "Entity '%s' already defined" % end.name)
> if cname in self._entity_dict:
> raise QAPIExprError(ent.info,
> "Entity '%s' collides with '%s'"
> % (ent.name, self._entity_dict[cname].name))
>
> where _lookup_entity() is back to the much simpler version:
>
> ent = self._entity_dict.get(cname)
> if ent and ent.name != cname
...because ent.name does not have to match cname.
> ent = None
>
> lookup_type() becomes:
>
> def lookup_type(self, name, typ=QAPISchemaType):
> return self._lookup_entity(name, typ)
I don't know that lookup_type needs an optional parameter.
>
> and the third caller _make_implicit_object_type() goes from
>
> if not self.lookup_entity(name, QAPISchemaObjectType):
>
> to
>
> if not self.lookup_type(name, QAPISchemaObjectType):
And a plain lookup_type(name) is what we should have been using here all
along.
>
> Another possible improvement is hiding the case-folding logic in
> methods. Have a QAPISchemaEntity.c_namecase() that returns
> self.c_name().lower(). Overwrite it in QAPISchemaEvent to return
> .upper() instead. Use it to make _def_entity() less ugly.
>
> Probably not worthwhile unless more uses of .c_namecase() pop up.
I kind of like it. I think I'll propose a 26.5 that implements that,
then redo 27 on top of it (without reposting the entire series).
>
> Second approach: don't use _entity_dict for clash detection! Have a
> second dictionary.
I debated about doing that the first time around. But it's probably a
bit easier to follow, so I think I'll do it.
>> @@ -1390,7 +1414,8 @@ class QAPISchema(object):
>>
>> def visit(self, visitor):
>> visitor.visit_begin(self)
>> - for (name, entity) in sorted(self._entity_dict.items()):
>> + for entity in sorted(self._entity_dict.values(),
>> + key=attrgetter('name')):
>> if visitor.visit_needed(entity):
>> entity.visit(visitor)
>> visitor.visit_end()
>
> Cool trick.
and with two dicts, I wouldn't need this trick.
>> +++ b/tests/qapi-schema/command-type-case-clash.err
>> @@ -0,0 +1 @@
>> +tests/qapi-schema/command-type-case-clash.json:3: Entity 'foo' collides
>> with 'Foo'
>
> The message's location is foo's. An additional message "'Foo' defined
> here" would be nice. Just an idea, could be done later.
The way we currently generate a single line message is by raising a
python exception. I guess we could rework things to build up lines at a
time, then finally raise the complete multi-line message on a pre-formed
string, rather than the current building the message as a parameter to
the exception constructor. But any changes on this front will have to
wait for later.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v12 20/36] blkdebug: Avoid '.' in enum values, (continued)
[Qemu-devel] [PATCH v12 27/36] qapi: Forbid case-insensitive clashes, Eric Blake, 2015/11/18
Re: [Qemu-devel] [PATCH v12 27/36] qapi: Forbid case-insensitive clashes, Eric Blake, 2015/11/18
[Qemu-devel] [PATCH] fixup! qapi: Forbid case-insensitive clashes, Eric Blake, 2015/11/18
Re: [Qemu-devel] [PATCH v12 27/36] qapi: Forbid case-insensitive clashes, Markus Armbruster, 2015/11/19
[Qemu-devel] [PATCH v12 19/36] blkdebug: Merge hand-rolled and qapi BlkdebugEvent enum, Eric Blake, 2015/11/18