On 11/18/2015 10:08 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> 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
