On 10/02/2015 08:00 AM, Markus Armbruster wrote: > Eric Blake <[email protected]> writes: > >> With the previous commit, we have two different locations for >> detecting member name clashes - one at parse time, and another >> at QAPISchema*.check() time. Consolidate some of the checks >> into a single place, which is also in line with our TODO to >> eventually defer all of the parse time semantic checking into >> the newer schema code. The check_member_clash() function is >> no longer needed. >> >> The wording of several error messages has changed, but in many >> cases feels like an improvement rather than a regression. The >> recent change to avoid an assertion failure when a flat union >> branch name collides with its discriminator name is also > > Which patch was that again?
v7a 6/18, http://repo.or.cz/qemu/ericb.git/commit/ede42e8e [uggh - gnu.org archives are still quite lagging, or I'd point to a mail] > >> handled nicely by this code; but there is more work needed >> before we can detect all collisions in simple union branch >> names done by the old code. >> >> No change to generated code. >> >> Signed-off-by: Eric Blake <[email protected]> >> >> @@ -611,11 +590,6 @@ def check_union(expr, expr_info): > # If the discriminator names an enum type, then all members > # of 'data' must also be members of the enum type, which in turn > # must not collide with the discriminator name. > > Should this comment be updated for the code removal below? Oh, right, revert both hunks of the earlier commit since we are now doing it in a less hacky manner. >> + v.check(schema, info, self.tag_member.type, vseen, >> + self.tag_name is not None) >> >> >> class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): >> def __init__(self, name, typ, owner): >> QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner) >> >> - def check(self, schema, info, tag_type, seen): >> + def check(self, schema, info, tag_type, seen, flat): >> QAPISchemaObjectTypeMember.check(self, schema, info, [], seen) >> assert self.name in tag_type.values >> + if flat: > > Any way to avoid the conditional? Not that I could come up with when I first tried, but it's been a while, so I can try again. I remember running into an assertion failure about nested ObjectType.check() calls if I did it unconditionally, since we allow the following: { 'struct': 'Fork', 'data': { '*fork': 'Union' } } { 'union': 'Union', 'data': { 'branch': 'Fork' } } We must NOT call self.type.check() on Fork (because we are not embedding the members of Fork into the simple union) - but then again, what we are really doing in the QAPISchema is visiting 'branch':':obj-Union-branch-wrapper', not 'Fork'. And then there's alternates, where branches aren't necessarily QAPISchemaObjectType in the first place. > > I've tried hard to make simple unions mere sugar for flat ones. There > are a few special cases left where we need to distinguish the two, but > they're all marked TODO. > > Can we have a brief comment explaing what we're checking here? We > generally don't have such comments in check() methods so far. Sort of > okay as long as they merely assert, but you're now starting to move > semantic analysis into them, which raises the bar. Yeah, I agree with adding more documentation about the checks. > >> + self.type.check(schema) > > Uh, careful. > > It's always okay for a check() method to call the check() of a child in > the abstract syntax tree, e.g. QAPISchemaObjectType.check() calling > m.check() for its members. A tree has no cycles. > > Calling it on anything else requires a non-trivial correctness argument. > Example: QAPISchemaObjectType.check() calls self.base.check(). Okay, > because no type may be a base of itself, and therefore a cycle would be > an error. The code takes care to detect cycles. > > I think the correctness argument here would be "no type contain itself > as variant member, and therefore a cycle would be an error." Do we > detect cycles? Not in this patch, but the assertion failure I ran into is the very reason I wrote a cycle detection patch next (11/12 in this series); maybe I should rebase that patch first? But you are correct that a flat union must not include itself as one of the branch types. I originally tried to test that in v5 3/46; your review convinced me it is no different than any other branch type that injects the same QMP members into the flat union, so I revamped the test in v6 to be a self-inheritance test instead, and then we dropped it after v7. (Search for "fork" in https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05733.html) > >> + assert isinstance(self.type.members, list) > > Implied by QAPISchemaObjectType.check()'s postcondition. Feels > redundant here, in particular since you iterate over it next anyway. Probably redundant, but I added it when I hit a cycle loop, to make sure that I wasn't still hitting a cycle (the 'if flat:' condition was the only way I could figure at the time to ensure I didn't hit this assertion). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
