Re: [Qemu-devel] [PATCH v7 11/14] qapi: Move duplicate member checks to schema check()

[Eric Blake]

On 10/12/2015 09:53 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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 move all of the parse time semantic checking into
>> the newer schema code.  The check_member_clash() function is
>> no longer needed.
>>

>> +    def check(self, schema, info, tag_type, seen, flat):
>>          QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
>>          assert self.name in tag_type.values
>> +        if flat:
>> +            # For flat unions, check that each QMP member does not
>> +            # collide with any non-variant members. No type can
>> +            # contain itself as a flat variant
>> +            self.type.check(schema)
>> +            assert not self.type.variants    # not implemented
>> +            for m in self.type.members:
>> +                if m.c_name() in seen:
>> +                    raise QAPIExprError(info,
>> +                                        "Member '%s' of branch '%s' 
>> collides "
>> +                                        "with %s"
>> +                                        % (m.name, self.name,
>> +                                           seen[m.c_name()].describe()))
> 
> If our data structures were right, we wouldn't need the conditional.
> 

Okay, you've made a good point. The only conditional that is appropriate
here is 'if not alternate', because you are right that flat and simple
unions should behave identically (and merely that simple unions won't
detect any collisions, because flattening their implicit struct with a
single 'data' member shouldn't introduce a conflict in QMP).


> So far the theory, now practice: if I hack the code to test "not
> alternate" (i.e. case 1 and 2) instead of "flat union" (just case 1),
> "make check" passes except for union-clash-data.json.  There, we now
> report a clash we didn't report before:
> 
>     union-clash-data.json:6: Member 'data' of branch 'data' collides with 
> 'data' (branch of TestUnion)

Hmm, I'll have to investigate before posting v8. At one point, I wired
in debug statements into the generator to show the contents of seen[]
through each call to check(), to see where collisions are coming from;
looks like I get to play with that again.

> 
> The FIXME in union-clash-data.json actually asks for an error, because
> 'data' clashes with our stupid filler to avoid empty unions.  But that's
> not what this error message reports!  I think what happens is this:
> QAPISchemaObjectTypeVariant.check() adds the case name self.name (here:
> 'data') to seen.  When we then check the case's 'data': 'int' member, it
> finds the with the case name, and reports a clash.  I can't see why it
> should.
> 
> This clashing business is awfully confusing, because we have to consider
> (flat unions, simple unions, alternates) times (QMP, C).
> 
> It would be simpler if we could make C clash only when QMP clashes.

If a QMP clash always caused a C clash, life would be a bit simpler. We
aren't going to get there (because in a flat union, hiding the members
of the branch type behind a single pointer means those members don't
clash with any C names of the overall union), but I can certainly try to
make the comments explain what is going on.

> 
> We might want to separate out alternates.  Dunno.

And to some extent, subset C already does some of that separation
(because I try to convert from 'FooKind type' to 'qtype_code type'
without even creating an implicit 'FooKind' enum).  It sounds like
you're okay with any well-documented TODO warts related to alternates
for the purposes of this subset B, especially if I can then clean those
warts back up in subset C.  But what v8 of subset B needs to avoid is
any warts based on simple vs. flat unions.  I can live with that.

I'm still trying to figure out if hoisting the kind=>type rename into
subset B makes life easier or harder (it certainly causes me more rebase
churn, but that's not necessarily a bad thing).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature