| Message ID | 1446791754-23823-23-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > From: Markus Armbruster <armbru@redhat.com> > > Reduce the ugly flat union / simple union conditional by doing just > the essential work here, namely setting self.tag_member. > Move the rest to callers. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Message-Id: <1446559499-26984-7-git-send-email-armbru@redhat.com> > [rebase to earlier changes that moved tag_member.check() of > alternate types, and tweak commit title and wording] > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v10: redo closer to Markus' original proposal > v9: new patch > --- > scripts/qapi.py | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 2a73b2b..e057408 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -988,9 +988,10 @@ class QAPISchemaObjectType(QAPISchemaType): > for m in self.local_members: > m.check(schema) > m.check_clash(seen) > + self.members = seen.values() > if self.variants: > self.variants.check(schema, seen) > - self.members = seen.values() > + assert self.variants.tag_member in self.members > > def is_implicit(self): > # See QAPISchema._make_implicit_object_type() > @@ -1052,8 +1053,6 @@ class QAPISchemaObjectTypeVariants(object): > def check(self, schema, seen): > if self.tag_name: # flat union > self.tag_member = seen[self.tag_name] My patch has: - if self.tag_name: # flat union + if not self.tag_member: # flat union self.tag_member = seen[self.tag_name] Any particular reason for dropping it? I like my change, because I feel it makes the assignment's purpose more obvious: ensure .tag_member is set. > - if seen: > - assert self.tag_member in seen.itervalues() > assert isinstance(self.tag_member.type, QAPISchemaEnumType) > for v in self.variants: > v.check(schema, self.tag_member.type)
On 11/09/2015 05:38 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> From: Markus Armbruster <armbru@redhat.com> >> >> Reduce the ugly flat union / simple union conditional by doing just >> the essential work here, namely setting self.tag_member. >> Move the rest to callers. >> >> @@ -1052,8 +1053,6 @@ class QAPISchemaObjectTypeVariants(object): >> def check(self, schema, seen): >> if self.tag_name: # flat union >> self.tag_member = seen[self.tag_name] > > My patch has: > > - if self.tag_name: # flat union > + if not self.tag_member: # flat union > self.tag_member = seen[self.tag_name] > > Any particular reason for dropping it? Not really; I couldn't find anything in later patches that cared either way. > > I like my change, because I feel it makes the assignment's purpose more > obvious: ensure .tag_member is set. Sure, we'll go with your approach.
diff --git a/scripts/qapi.py b/scripts/qapi.py index 2a73b2b..e057408 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -988,9 +988,10 @@ class QAPISchemaObjectType(QAPISchemaType): for m in self.local_members: m.check(schema) m.check_clash(seen) + self.members = seen.values() if self.variants: self.variants.check(schema, seen) - self.members = seen.values() + assert self.variants.tag_member in self.members def is_implicit(self): # See QAPISchema._make_implicit_object_type() @@ -1052,8 +1053,6 @@ class QAPISchemaObjectTypeVariants(object): def check(self, schema, seen): if self.tag_name: # flat union self.tag_member = seen[self.tag_name] - if seen: - assert self.tag_member in seen.itervalues() assert isinstance(self.tag_member.type, QAPISchemaEnumType) for v in self.variants: v.check(schema, self.tag_member.type)