On 11/11/2015 06:42 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes:
> Your patch actually does two things: > > 1. Extend the "object type cannot contain itself" check to cover its > variant types in addition to base type. This is the two-liner change > to QAPISchemaObjectTypeVariants.check(). > > 2. Extend the "object type cannot contain members with clashing names" > to cover variant members. > > They're related: you need the former's side effect to compute .members > to be able to do the latter. > > Doing it in two separate patches *might* make explaining it in the > commit message easier. See below. Interesting observation. I can split if you still think it is worth it; otherwise, I think your wording makes both changes obvious: > Let me have a stab at the commit message of the *unsplit* patch, use as > you see fit: > > qapi: Check for collisions involving variant members > > Right now, our ad hoc parser ensures that we cannot have a flat union > that introduces any members that would clash with non-variant members > inherited from the union's base type (see flat-union-clash-member.json). > We want QAPISchemaObjectType.check() to make the same check, so we can > later reduce some of the ad hoc checks. > > We already have a map 'seen' of all non-variant members. We still need > to check for collisions between each variant type's members and the > non-variant ones. > > To know the variant type's members, we need to call > variant.type.check(). This also detects when a type contains itself in > a variant, exactly like the existing base.check() detects when a type > contains itself as a base. If I split, this paragraph would go in the first patch (adding the .check() for cycle detection). > > Slight complication: an alternate's variant can have arbitrary type, but > only an object type's check() may be called outside QAPISchema.check(). > We could either skip the call for variants of alternates, or skip it for > non-object types. Do the latter, because it's easier. I actually have some churn on this; later in 22/28, I'm stuck making Variants.check() do something for unions only, and had to reintroduce an 'if seen:' conditional, at which point I moved the v.type.check() back to unions only. I guess your review of that patch may determine whether I minimize churn back here, or add a 'for now' phrase to the commit message, or just not worry about it. > > Then we can call each variant member's check_clash() with the > appropriate 'seen' map. Since members of different variants can't > clash, We have to clone a fresh seen for each variant. Wrap this in the > new helper method QAPISchemaObjectTypeVariants.check_clash(). > > Note that cloning 'seen' inside Variants.check_clash() > resembles the one we just removed from Variants.check(); the > difference here is that we are now checking for clashes > among the qapi members of the variant type, rather than for > a single clash with the variant tag name itself. > > Note that by construction collisions can't actually happen for simple > unions: each variant's type is a wrapper with a single member 'data', > which will never collide with the only non-variant member 'type'. > > For alternates, there's nothing for a variant object type's members to > clash with, and therefore no need to call variants.check_clash(). > > No change to generated code. > -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
