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: 4206 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).
>>

>> @@ -873,8 +856,30 @@ class QAPISchemaEnumType(QAPISchemaType):
>>          self.values = values
>>          self.prefix = prefix
>>
>> +    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?

D'oh; rebase churn reordering patches.

> 
>> +            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.

Sure, that sounds slightly better.

> 
> 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.

Are you talking about _pretty_owner()? Or the actual describe() that
calls _pretty_owner()?

> Rename it to match this one?
> 
>> +
>>      def check(self, schema):
>> -        assert len(set(self.values)) == len(self.values)
>> +        # Check for collisions on the generated C enum values
>> +        seen = {c_enum_const(self.name, 'MAX'): '(automatic MAX)'}
>> +        for value in self.values:
>> +            c_value = c_enum_const(self.name, value)
>> +            if c_value in seen:
>> +                raise QAPIExprError(self.info,
>> +                                    "%s '%s' clashes with '%s'"
>> +                                    % (self._describe(schema), value,
>> +                                       seen[c_value]))
>> +            seen[c_value] = value
> 
> Loop body is very similar to QAPISchemaObjectTypeMember.check_clash().
> Differences:
> 
> * c_enum_const(enum_name, member_name) vs. c_name(member_name).upper()
> 
>   This isn't really a difference, because the former returns the latter
>   prefixed by a string that doesn't vary in the loop.

Well, it _is_ a difference if c_name() munges differently than
c_enum_const() (the whole question of whether 'OneTwo' and 'One-Two' are
detected as clashes); but then again, I have the patches that try to
rework c_enum_const() to use just c_name().upper().

> 
>   One could argue that using c_enum_const() lets us abstract from what
>   it does, but that's an illusion.  We rely on it when we generate union
>   members called c_name(member_name) without checking for collisions
>   again.
> 
>   By the way, c_enum_const(self.name, value, self.prefix) would be more
>   correct.  Doesn't matter here, of course.
> 
>   Therefore, I'd be very much tempted to use c_name(member_name).upper()
>   here as well.
> 
> * The error message.  But I suspect the same "Member '%s' clashes with
>   '%s'" could do for both.
> 
> If I'm right and we can drop the differences, the common code could
> become a helper function.

I'll play with it and see what pops out.

-- 
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 --]