On 11/03/2015 04:06 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes: > >> The implementation of QAPISchemaObjectTypeMember.check() always >> adds the member currently being checked to both the all_members >> and seen parameters. > > QAPISchemaObjectTypeMember.check() does four things: > > 1. Compute self.type > > Precondition: all types are defined.
Correct, unchanged by this patch.
>
> 2. Accumulate members
>
> all_members serves as accumulator.
>
> We'll see that its only actual use is the owning object type's
> check(), which uses it to compute self.members.
This patch changes it to use seen.values(), which (once you use an
OrderedDict() instead of plain {}) is identical to all_members.
>
> 3. Check for collisions
>
> This works by accumulating names in seen. Precondition: seen
> contains the names seen so far.
>
> Note that this part uses seen like a set. See 4.
Unchanged by this patch; but see also 2/4 and 3/4.
>
> 4. Accumulate a map from names to members
>
> seen serves as accumulator.
>
Unchanged by this patch.
> We'll see that its only actual user is the owning object type's
> variants.check(), which uses it to compute variants.tag_member from
> variants.tag_name.
>
>> However, the three callers of this method
>> pass in the following parameters:
>>
>> QAPISchemaObjectType.check():
>> - all_members contains all non-variant members seen to date,
>> for use in populating self.members
>> - seen contains all non-variant members seen to date, for
>> use in checking for collisions
>
> Yes, and:
>
> - we're calling it for m in self.local_members
> - before the loop, all_members and seen are initialized to the inherited
> non-variant members
> - after the loop, they therefore contain all non-variant members
>
> This caller uses all four things done by QAPISchemaObjectType.check():
>
> 1. Compute m.type
Unchanged by this patch.
> 2. Accumulate non-variant members
Whether the accumulation is done via all_members (pre-patch) or by
seen.values() (post-patch), this step is still done.
> 3. Check for collisions among non-variant members
> Before the loop, seen contains the inherited members, which don't
> collide (self.base.check() ensures that). The loop adds the local
> members one by one, checking for collisions.
Unchanged by this patch.
> 4. Accumulate a map from names to non-variant members
> Similar argument to 3.
Unchanged by this patch.
>
>> QAPISchemaObjectTypeVariant.check():
>
> Do you mean QAPISchemaObjectVariants.check()?
QAPISchemaObjectTypeVariants.check() calls
QAPISchemaObjectTypeVariant.check() for each variant, but with a fresh
copy of seen. We'll later need to expand this copy of seen (patch 2/4),
but for this patch its use is unchanged - we are appending a single
value (the tag value) which is wrong, but no one cares that we appended
it because it was a copy. Patch 3/4 fixes to not append to it.
>
>> - all_members is a throwaway empty list
>> - seen is a throwaway dictionary created as a copy by
>> QAPISchemaObjectVariants.check() (since the members of
>> one variant cannot collide with those from another), for
>> use in checking for collisions (technically, we no longer
>> need to check for collisions between tag values and QMP
>> key names, but that's a cleanup for another patch)
>>
>> QAPISchemaAlternateType.check():
>> - all_members is a throwaway empty list
>> - seen is a throwaway empty dict
>
> I'm afraid you're omitting a few steps here, and I think you missed
> QAPISchemaObjectVariants.check()'s self.tag_member.check().
There is no self.tag_member.check(), any more; rather, my earlier
patches have reworked things so that tag_member is checked by the owner
(a flat union's base type, a simple union's local_members, or directly
in QAPISchemaAlternateType prior to calling Variants.check()). I guess
that's a pitfall of seeing this patch without my rework of 5/17 to
address your comments there.
>> Therefore, in the one case where we care about all_members
>> after seen has been populated, we know that it contains the
>> same members as seen.values(); changing seen to be an
>> OrderedDict() is sufficient to pick up this information with
>> one less parameter being passed around.
>
> I believe the first step should be dropping the obsolete check for
> collision of tag value with non-variant members. I believe this should
> do:
>
> @@ -1059,8 +1059,7 @@ class QAPISchemaObjectTypeVariants(object):
> self.tag_member.check(schema, members, seen)
> assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> for v in self.variants:
> - vseen = dict(seen)
> - v.check(schema, self.tag_member.type, vseen)
> + v.check(schema, self.tag_member.type, {})
Close, but not quite. It should do:
+ cases = {}
for v in self.variants:
vseen = dict(seen)
- v.check(schema, self.tag_member.type, vseen)
+ v.check(schema, self.tag_member.type, vseen, cases)
coupled with this in QAPISchemaObjectTypeVariant:
- def check(self, schema, tag_type, seen):
- QAPISchemaObjectTypeMember.check(self, schema, [], seen)
+ def check(self, schema, tag_type, seen, cases):
+ QAPISchemaObjectTypeMember.check(self, schema, [], cases)
so that we are now checking collisions between tag values, rather than
cases. But that's what I did in patch 3/4. And we still need seen
passed to Variant.check(), because that's the checking added in 2/4.
Okay, you've convinced me - when I post v9, I'll reorder these four
patches to put 3/4 first.
>
>
> class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>
> Then only one caller about 2-4., namely QAPISchemaObjectType.check().
> Simplify radically: move 2-4. to the caller that cares, drop parameters
> all_members and seen.
Nope - because seen (well, a copy of seen) is still important to patch 2/4.
>
> Still to do then: non-variant member collision checking. Factor out
> 3. into a helper function, use it for non-variant members.
Factoring into a helper function is done in 4/4. I can try and
rearrange that earlier, too.
>
> What do you think?
>
Can you at least look at 2, 3, and 4 to see where I'm headed, and then I
can rearrange things for the v9 spin? We're probably talking a bit past
each other, with the same end goal, but a muddle in the middle of how to
get there.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
