Markus Armbruster <arm...@redhat.com> writes: > Eric Blake <ebl...@redhat.com> writes: > >> Right now, our ad hoc parser ensures that we cannot have a >> flat union that introduces any qapi member names that would >> conflict with the non-variant qapi members already present >> 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 ensure that all branches of a flat union are qapi >> structs with no variants, at which point those members appear >> in the same JSON object as all non-variant members. And we >> already have a map 'seen' of all non-variant members. All >> we need is a new QAPISchemaObjectTypeVariants.check_clash(), >> which clones the seen map then checks for clashes with each >> member of the variant's qapi type. >> >> Note that the clone of 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. >> >> In general, a type used as a branch of a flat union cannot >> also be the base type of the flat union, so even though we are >> adding a call to variant.type.check() in order to populate >> variant.type.members, this is merely a case of gaining >> topological sorting of how types are visited (and type.check() >> is already set up to allow multiple calls due to base types). > > Yes, a type cannot contain itself, neither as base nor as variant. > > We have tests covering attempts to do the former > (struct-cycle-direct.json, struct-cycle-indirect.json). As far as I can > see, we don't have tests covering the latter. Do we catch it? > >> For simple unions, the same code happens to work by design, >> because of our synthesized wrapper classes (however, the >> wrapper has a single member 'data' which will never collide >> with the one non-variant member 'type', so it doesn't really >> matter). >> >> There is no impact to alternates, which intentionally do not >> need to call variants.check_clash() (there, at most one of >> the variant's branches will be an ObjectType, and even if one >> exists, we are not inlining the qapi members of that object >> into a parent object, the way we do for unions).
Yes. QAPISchemaObjectTypeVariants.check_clash() checks for each variant's members clashing with other members in the same name space. For alternates, there are no such other members. That said, should we add a comment to QAPISchemaAlternateType.check()? Perhaps: # Not calling self.variant.check_clash(), because there's # nothing to clash with. >> No change to generated code. >> >> Signed-off-by: Eric Blake <ebl...@redhat.com> > > Patch looks good.