Eric Blake <ebl...@redhat.com> writes: > On 11/04/2015 04:11 PM, Eric Blake wrote: >> On 11/04/2015 12:01 PM, Markus Armbruster wrote: >>> 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 QAPISchema*.check() to make the same check, so we can >>>> later reduce some of the ad hoc checks. >>>> >> > >>> Why can't we simply add the new code to QAPISchemaObjectType.check()? >> >> We could, but we'd need a nested for-loop: >> >> for v in variants.variants: >> v.type.check(schema) >> assert not v.type.variants >> vseen = dict(seen) >> for m in v.type.members: >> m.check_clash(seen)
Looks clear enough to me. >>> The rest of the clash checking is already there... >> >> You do have a point there. I guess it wouldn't be the end of the world >> to have the loop nesting be explicit rather than indirect through the >> intermediate Variants.check(). Hiding loop nesting behind method calls doesn't make code simpler :) Methods help when they're useful abstractions. For instance, hiding the details of checking a member m behind m.check() is good. But here, we're checking relations between members. >> I'll play with it; and depending on what I do, that may mean I don't >> have to munge your other patches quite so heavily. > > And of course, as soon as I hit send, I had another thought - your > patches already added Member.check_clash() called separately from the > .check() chain; maybe I am better off adding a Variants.check_clash() > separate from the .check() chain, rather than trying to cram the whole > nested loop directly in ObjectType.check(). Matter of taste, can't tell without trying, use your judgement.