| Message ID | 1446504092-29008-2-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. 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?
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> writes: > 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. I'm assuming this patch is based on [PATCH v8 06/17] qapi-types: Consolidate gen_struct() and gen_union() which has def check(self, schema, members, seen): if self.tag_name: # flat union self.tag_member = seen[self.tag_name] elif seen: # simple union assert self.tag_member in seen.itervalues() else: # alternate ---> self.tag_member.check(schema, members, seen) in QAPISchemaObjectTypeVariants. >>> 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. A sanity check "no collisions between tag values" doesn't hurt, but I'd simply rely on the tag type's .check(). The tag type is an enumeration type, its check() ensures the enumeration values are distinct. For variants, checking the tag value is a is a value of the tag type suffices to ensure they don't collide. > 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. Yes, we're almost certainly headed in the same direction. But I got thoroughly confused and lost in the details on the way, so I had to hack things up myself to clear my head. The result looks nice to me, so I'll send it out in a jiffie in the hope you'll find it useful.
On 11/03/2015 07:02 AM, Markus Armbruster wrote: >>> 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. > > I'm assuming this patch is based on > [PATCH v8 06/17] qapi-types: Consolidate gen_struct() and gen_union() > which has > > def check(self, schema, members, seen): > if self.tag_name: # flat union > self.tag_member = seen[self.tag_name] > elif seen: # simple union > assert self.tag_member in seen.itervalues() > else: # alternate > ---> self.tag_member.check(schema, members, seen) > > in QAPISchemaObjectTypeVariants. That was true in v8, but not in my pending v9 which did essentially what your 7/7 did (move the tag_member.check() into Alternate.check() back in patch 5). At any rate, my first glance of your series shows that it is reasonable, so my task today is to spit out a v9 of my series, but using your seven patches in place of my four. > Yes, we're almost certainly headed in the same direction. But I got > thoroughly confused and lost in the details on the way, so I had to hack > things up myself to clear my head. The result looks nice to me, so I'll > send it out in a jiffie in the hope you'll find it useful. > Yes, it should make v9 easier when it includes your patches, because then you know what to look for :)
Eric Blake <eblake@redhat.com> writes: > On 11/03/2015 07:02 AM, Markus Armbruster wrote: > >>>> 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. >> >> I'm assuming this patch is based on >> [PATCH v8 06/17] qapi-types: Consolidate gen_struct() and gen_union() >> which has >> >> def check(self, schema, members, seen): >> if self.tag_name: # flat union >> self.tag_member = seen[self.tag_name] >> elif seen: # simple union >> assert self.tag_member in seen.itervalues() >> else: # alternate >> ---> self.tag_member.check(schema, members, seen) >> >> in QAPISchemaObjectTypeVariants. > > That was true in v8, but not in my pending v9 which did essentially what > your 7/7 did (move the tag_member.check() into Alternate.check() back in > patch 5). > > At any rate, my first glance of your series shows that it is reasonable, > so my task today is to spit out a v9 of my series, but using your seven > patches in place of my four. Buyer beware: I'm not sure my seven do everything your four do. >> Yes, we're almost certainly headed in the same direction. But I got >> thoroughly confused and lost in the details on the way, so I had to hack >> things up myself to clear my head. The result looks nice to me, so I'll >> send it out in a jiffie in the hope you'll find it useful. >> > > Yes, it should make v9 easier when it includes your patches, because > then you know what to look for :) I hope I'll be less easily confused going forward...
On 11/03/2015 09:19 AM, Markus Armbruster wrote: >>> I'm assuming this patch is based on >>> [PATCH v8 06/17] qapi-types: Consolidate gen_struct() and gen_union() >>> which has >>> >>> def check(self, schema, members, seen): >>> if self.tag_name: # flat union >>> self.tag_member = seen[self.tag_name] >>> elif seen: # simple union >>> assert self.tag_member in seen.itervalues() >>> else: # alternate >>> ---> self.tag_member.check(schema, members, seen) >>> >>> in QAPISchemaObjectTypeVariants. >> >> That was true in v8, but not in my pending v9 which did essentially what >> your 7/7 did (move the tag_member.check() into Alternate.check() back in >> patch 5). Caused me some churn in incorporating your patches, but not too bad. >> >> At any rate, my first glance of your series shows that it is reasonable, >> so my task today is to spit out a v9 of my series, but using your seven >> patches in place of my four. > > Buyer beware: I'm not sure my seven do everything your four do. Yours do the same as my patch 1. You also made a compelling argument for eliminating my patch 3 (the enum values are already collision-proof, therefore if we check that a variant name is in the enum, we don't need to check for collisions) - except that in my 10/17, when I get rid of the generated enum for alternates, I have to reinstate that check somewhere; but adding it directly to QAPISchemaAlternateType.check() at that point feels better. My patch 2 is still needed, but looks a bit nicer on top of some of your refactoring. And my patch 4 is still useful, as additional refactoring to share the code used by my patch 2 and your code for ObjectType.check(). We'll see how it all turns out in v9.
diff --git a/scripts/qapi.py b/scripts/qapi.py index 6b6ba5f..fff4adb 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -972,28 +972,26 @@ class QAPISchemaObjectType(QAPISchemaType): self.variants = variants self.members = None + # Finish construction, and validate that all members are usable def check(self, schema): assert self.members is not False # not running in cycles if self.members: return self.members = False # mark as being checked + seen = OrderedDict() if self._base_name: self.base = schema.lookup_type(self._base_name) assert isinstance(self.base, QAPISchemaObjectType) assert not self.base.variants # not implemented self.base.check(schema) - members = list(self.base.members) - else: - members = [] - seen = {} - for m in members: - assert c_name(m.name) not in seen - seen[m.name] = m + for m in self.base.members: + assert c_name(m.name) not in seen + seen[m.name] = m for m in self.local_members: - m.check(schema, members, seen) + m.check(schema, seen) if self.variants: - self.variants.check(schema, members, seen) - self.members = members + self.variants.check(schema, seen) + self.members = seen.values() def is_implicit(self): # See QAPISchema._make_implicit_object_type() @@ -1027,11 +1025,10 @@ class QAPISchemaObjectTypeMember(object): self.type = None self.optional = optional - def check(self, schema, all_members, seen): + def check(self, schema, seen): assert self.name not in seen self.type = schema.lookup_type(self._type_name) assert self.type - all_members.append(self) seen[self.name] = self @@ -1050,7 +1047,7 @@ class QAPISchemaObjectTypeVariants(object): self.tag_member = tag_member self.variants = variants - def check(self, schema, members, seen): + def check(self, schema, seen): if self.tag_name: # flat union self.tag_member = seen[self.tag_name] assert self.tag_member @@ -1067,7 +1064,7 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): QAPISchemaObjectTypeMember.__init__(self, name, typ, False) def check(self, schema, tag_type, seen): - QAPISchemaObjectTypeMember.check(self, schema, [], seen) + QAPISchemaObjectTypeMember.check(self, schema, seen) assert self.name in tag_type.values # This function exists to support ugly simple union special cases @@ -1090,8 +1087,8 @@ class QAPISchemaAlternateType(QAPISchemaType): def check(self, schema): seen = {} - self.variants.tag_member.check(schema, [], seen) - self.variants.check(schema, [], seen) + self.variants.tag_member.check(schema, seen) + self.variants.check(schema, seen) def json_type(self): return 'value'
The implementation of QAPISchemaObjectTypeMember.check() always adds the member currently being checked to both the all_members and seen parameters. 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 QAPISchemaObjectTypeVariant.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 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. Signed-off-by: Eric Blake <eblake@redhat.com> --- v9: new patch, split off from v8 7/17 --- scripts/qapi.py | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-)