Re: [Qemu-devel] [PATCH v8 16/18] qapi: Move duplicate enum value checks to schema check()

[Eric Blake]

On 10/13/2015 12:35 PM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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,
>> so for a decent error message, we have to determine if the enum
>> is implicit (and if so where the real collision lies).
>>
>> Testing this showed that the test union-bad-branch and
>> union-clash-branches were basically testing the same thing;
>> with the minor difference that the former clashes only in the
>> enum, while the latter also clashes in the C union member
>> names that would be generated. So delete the weaker test.
>>
>> No change to generated code.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>>

>> +++ b/scripts/qapi.py
>> @@ -527,7 +527,6 @@ def check_union(expr, expr_info):
>>      base = expr.get('base')
>>      discriminator = expr.get('discriminator')
>>      members = expr['data']
>> -    values = {'MAX': '(automatic)', 'KIND': '(automatic)'}
> 
> Stupid / tired question: I can see 'MAX' in the new code further down,
> but I can't see 'KIND'.  Why?  Was it perhaps covered by the previous
> patch?

MAX is covered by enum collisions.  'kind' is covered by the previous
patch's non-variant vs. branch-name collisions.

Hmm, maybe when respinning this I can simplify 15/18 to drop KIND from
this spot (because KIND is not part of the enum).


>>
>>      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:
>> +                # 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):
>> +                        description = "Alternate '%s' branch" % owner.name
>> +                    else:
>> +                        description = "Union '%s' branch" % owner.name
>> +                else:
>> +                    description = "Enum '%s' value" % self.name
> 
> Computing a reasonable description distracts from the checking job.
> Suggest to outline this into a private method.

Good idea.

>> +++ b/tests/qapi-schema/alternate-clash.err
>> @@ -1 +1 @@
>> -tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' member 'a_b' 
>> clashes with 'a-b'
>> +tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' branch 'a_b' 
>> clashes with 'a-b'
> 
> Our terminology isn't consistent: we use both "branch" and "case".  Not
> this patch's problem to fix.
> 
>> diff --git a/tests/qapi-schema/enum-clash-member.err 
>> b/tests/qapi-schema/enum-clash-member.err
>> index 48bd136..84030c5 100644
>> --- a/tests/qapi-schema/enum-clash-member.err
>> +++ b/tests/qapi-schema/enum-clash-member.err
>> @@ -1 +1 @@
>> -tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' member 'ONE' 
>> clashes with 'one'
>> +tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' value 'ONE' 
>> clashes with 'one'
> 
> I actually prefer calling ONE a member of MyEnum, because that leaves
> value for its actual value.
> 
> For what it's worth, the C standard also talks about "members of an
> enumeration".

Easy enough to fix.

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

signature.asc
Description: OpenPGP digital signature