[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8.5 1/4] qapi: Drop all_members parameter from
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v8.5 1/4] qapi: Drop all_members parameter from check() |
Date: |
Tue, 03 Nov 2015 12:06:59 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> 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.
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.
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.
4. Accumulate a map from names to members
seen serves as accumulator.
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
2. Accumulate non-variant members
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.
4. Accumulate a map from names to non-variant members
Similar argument to 3.
> QAPISchemaObjectTypeVariant.check():
Do you mean QAPISchemaObjectVariants.check()?
> - 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(). Let me go
through the remaining callers of QAPISchemaObjectTypeMember.check() real
slow.
* QAPISchemaObjectType.check() calls it via v.check(), which is a thin
wrapper that throws away 2. and asserts something of no interest here.
seen is a map from names to non-variant members. Therefore,
QAPISchemaObjectTypeMember.check() gets used here as follows:
1. Compute v.type
2. Thrown away
3. Check for collision of tag value with non-variant members
This is obsolete now.
4. Thrown away
* QAPISchemaAlternateType.check() calls it via v.check(), which is the
same thin wrapper. seen is {} here. Therefore:
1. Compute v.type
2. Thrown away
3. No-op
4. Thrown away
* QAPISchemaObjectTypeVariants.check() calls
self.tag_member.check(schema, members, seen)
where members and seen contain the non-variant members.
However, it gets called only when the owning type is an alternate, and
then members and seen are both empty. Therefore:
1. Compute v.type
2. Thrown away
3. No-op
4. Thrown away
> 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, {})
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.
Still to do then: non-variant member collision checking. Factor out
3. into a helper function, use it for non-variant members.
What do you think?
- Re: [Qemu-devel] [PATCH v8 07/17] qapi: Rework collision assertions, Markus Armbruster, 2015/11/02
- Re: [Qemu-devel] [PATCH v8 07/17] qapi: Rework collision assertions, Eric Blake, 2015/11/02
- Re: [Qemu-devel] [PATCH v8 07/17] qapi: Rework collision assertions, Eric Blake, 2015/11/02
- [Qemu-devel] [PATCH v8.5 0/4] rework of 7/17, Eric Blake, 2015/11/02
- [Qemu-devel] [PATCH v8.5 1/4] qapi: Drop all_members parameter from check(), Eric Blake, 2015/11/02
- Re: [Qemu-devel] [PATCH v8.5 1/4] qapi: Drop all_members parameter from check(),
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH v8.5 1/4] qapi: Drop all_members parameter from check(), Eric Blake, 2015/11/03
- Re: [Qemu-devel] [PATCH v8.5 1/4] qapi: Drop all_members parameter from check(), Markus Armbruster, 2015/11/03
- Re: [Qemu-devel] [PATCH v8.5 1/4] qapi: Drop all_members parameter from check(), Eric Blake, 2015/11/03
- Re: [Qemu-devel] [PATCH v8.5 1/4] qapi: Drop all_members parameter from check(), Markus Armbruster, 2015/11/03
- Re: [Qemu-devel] [PATCH v8.5 1/4] qapi: Drop all_members parameter from check(), Eric Blake, 2015/11/03
- [Qemu-devel] [PATCH 1/7] qapi: Drop obsolete tag value collision assertions, Markus Armbruster, 2015/11/03
- [Qemu-devel] [PATCH 2/7] qapi: Simplify QAPISchemaObjectTypeMember.check(), Markus Armbruster, 2015/11/03
- [Qemu-devel] [PATCH 7/7] qapi: QAPISchemaObjectTypeVariants.check(), Markus Armbruster, 2015/11/03
- [Qemu-devel] [PATCH 4/7] qapi: Fix up commit 7618b91's clash sanity checking change, Markus Armbruster, 2015/11/03
- [Qemu-devel] [PATCH 5/7] qapi: Eliminate QAPISchemaObjectType.check() variable members, Markus Armbruster, 2015/11/03