Re: [Qemu-devel] [PATCH v11 27/28] qapi: Move duplicate enum value checks to schema check()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2715 bytes --]

On 11/12/2015 08:46 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Similar to the previous commit, move the detection of a collision
>> in enum values from parse time to QAPISchemaEnumType.check().
>> This happens to also detect collisions in union branch names
>> mapping to the same enum value, even when the names do not
>> collide case-wise.  So for a decent error message, we have to
>> determine if the enum is implicit (and if so where the real
>> collision lies).
>>

>> +    def _describe(self, schema):
>> +        # If the enum is implicit, report the error on behalf of
>> +        # the union or alternate that triggered the enum
>> +        if self.is_implicit():
>> +            owner = schema.lookup_type(self.name[:-4])
>> +            assert owner
>> +            if isinstance(owner, QAPISchemaAlternateType):
>> +                return "Alternate '%s' branch" % owner.name
> 
> Didn't we just drop this kind of implicit enum?
> 
>> +            else:
>> +                return "Union '%s' branch" % owner.name
>> +        else:
>> +            return "Enum '%s' value" % self.name
> 
> I like to call it "member" rather than value, because it avoids
> confusion with the numeric value of the C enumeration constant generated
> for it.
> 
> The conditional isn't exactly elegant, but it would do.  I'm not 100%
> convinced we need it, though.  self.info already points to whatever
> defined this enum, either an explicit enum definition, or a simple union
> definition.  How do the error messages come out if we dumb down to
> "Member '%s'"?
> 
> A method with a similar purpose exists in QAPISchemaObjectTypeMember,
> but it's spelled describe().  It's used only from within the class.
> Rename it to match this one?

[as much notes to myself as anything else...]

We chatted some on IRC, and had some more ideas, thus I will delay
patches 26-28 to a later date while posting the rest of v12.  Among
those ideas
 - getting rid of the enum _MAX clash will simplify things (to be done
in v12)
 - create a new QAPISchemaMember class as the superclass of
QAPISchemaObjectTypeMember, and have enum members be instances of this
type rather than a straight string (at which point the describe() method
and collision checking lives in the superclass, while the associated
type only lives in the subclass). Less code duplication that way
 - the idea of multiline error messages, only when info !=
member.owner.info; as in:
foo.json:4: Member 'one' clashes with 'One'
foo.json:2: Member 'one' defined here

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]