Re: [Qemu-devel] [PATCH v8.5 1/4] qapi: Drop all_members parameter from check()

[Eric Blake <eblake@redhat.com>]

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

On 11/03/2015 04:06 AM, Markus Armbruster wrote:
> Eric Blake <eblake@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


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