[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v11 27/28] qapi: Move duplicate enum value check
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v11 27/28] qapi: Move duplicate enum value checks to schema check() |
Date: |
Thu, 12 Nov 2015 09:08:39 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 11/12/2015 08:46 AM, 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
>> 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
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v11 22/28] qapi: Simplify visiting of alternate types, (continued)
- [Qemu-devel] [PATCH v11 28/28] qapi: Detect base class loops, Eric Blake, 2015/11/11
- [Qemu-devel] [PATCH v11 23/28] qapi: Fix alternates that accept 'number' but not 'int', Eric Blake, 2015/11/11
- [Qemu-devel] [PATCH v11 27/28] qapi: Move duplicate enum value checks to schema check(), Eric Blake, 2015/11/11
- [Qemu-devel] [PATCH v11 18/28] qerror: more error_setg() usage, Eric Blake, 2015/11/11
- Re: [Qemu-devel] [PATCH v11 18/28] qerror: more error_setg() usage, Andreas Färber, 2015/11/11
- Re: [Qemu-devel] [PATCH v11 18/28] qerror: more error_setg() usage, Markus Armbruster, 2015/11/11
- Re: [Qemu-devel] [PATCH v11 18/28] qerror: more error_setg() usage, Eric Blake, 2015/11/11
- Re: [Qemu-devel] [PATCH v11 18/28] qerror: more error_setg() usage, Markus Armbruster, 2015/11/11
- Re: [Qemu-devel] [PATCH v11 18/28] qerror: more error_setg() usage, Eric Blake, 2015/11/11
[Qemu-devel] [PATCH v11 21/28] qapi: Convert qtype_code into qapi enum type, Eric Blake, 2015/11/11