Re: [Qemu-devel] [PATCH v10 28/30] qapi: Detect collisions in C member names

[Eric Blake]

On 11/09/2015 08:17 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Detect attempts to declare two object members that would result
>> in the same C member name, by keying the 'seen' dictionary off
>> of the C name rather than the qapi name.  It also requires passing
>> info through the check_clash() methods.
>>
>> This addresses a TODO and fixes the previously-broken
>> args-name-clash test.  The resulting error message demonstrates
>> the utility of the .describe() method added previously.  No change
>> to generated code.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>>
>> ---

>> +++ b/scripts/qapi.py
>> @@ -975,21 +975,24 @@ class QAPISchemaObjectType(QAPISchemaType):
>>          seen = OrderedDict()
>>          if self._base_name:
>>              self.base = schema.lookup_type(self._base_name)
>> -            self.base.check_clash(schema, seen)
>> +            self.base.check_clash(schema, self.info, seen)
> 
> Note that if this one reports an error for self.info, something went
> wrong.  We first run self.base.check(), which we survive only when
> self.base doesn't have clashing members.  Would be easier to see if
> self.base.check() wasn't hiding in self.base.check_clash().

Hmm. I could pass None as a way of asserting that it won't fail.  But I
think that's a bit more confusing, so I'll probably just leave it alone.

>> -    def check_clash(self, schema, seen):
>> +    # Check that the members of this type do not cause duplicate JSON 
>> fields,
>> +    # and update seen to track the members seen so far. Report any errors
>> +    # on behalf of info, which is not necessarily self.info
> 
> Do we actually need info != self.info?  If yes, test case?  If no,
> should the comment be adjusted?

The comment is correct.  For a flat union, if we have a variant
associated with type 'Foo', and type 'Foo' has no clashes itself, but
one of its members duplicates a member already present from the base of
union, then we are calling Foo.check_clash(schema, Union.info, seen), so
that the error message points to the location of Union (rather than Foo)
as the place in the .json file introducing the clash.

flat-union-clash-member.json tests this; it's just that we first have to
remove ad hoc testing before we can trigger this case.  It's on my
queue, just not in this particular posting of subset C:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg07000.html


>> @@ -1035,10 +1038,13 @@ class QAPISchemaObjectTypeMember(object):
>>          self.type = schema.lookup_type(self._type_name)
>>          assert self.type
>>
>> -    def check_clash(self, seen):
>> -        # TODO change key of seen from QAPI name to C name
>> -        assert self.name not in seen
>> -        seen[self.name] = self
>> +    def check_clash(self, info, seen):
>> +        name = c_name(self.name)
> 
> I'd call it cname.  Matter of taste, of course.

Sure, that works.  I first tried calling it c_name, but then python
thought I was trying to redefine the global c_name().

>>
>> -    def check_clash(self, schema, seen):
>> +    def check_clash(self, schema, info, seen):
>>          for v in self.variants:
>>              # Reset seen map for each variant, since qapi names from one
>>              # branch do not affect another branch
>> -            v.type.check_clash(schema, dict(seen))
>> +            v.type.check_clash(schema, info, dict(seen))
> 
> Since we can't inherit variant members, info is always the owning object
> type's info.

Exactly. We want to output the passed-in info, and NOT v.type.info, so
that errors are reported on behalf of the union that owns the variant,
rather than at the location of the variant's type which independently
passed .check() and therefore has no internal collisions.

And someday we might be able to inherit from objects with variants, but
that's a lot further down the pipeline (if at all).


>> +++ b/tests/qapi-schema/args-name-clash.json
>> @@ -1,5 +1,5 @@
>>  # C member name collision
>> -# FIXME - This parses, but fails to compile, because the C struct is given
>> -# two 'a_b' members.  Either reject this at parse time, or munge the C names
>> -# to avoid the collision.
>> +# Reject members that clash when mapped to C names (we would have two 'a_b'
>> +# members). It would also be possible to munge the C names to avoid the
>> +# collision, but unlikely to be worth the effort.
> 
> I'd drop the second sentence.

Done.

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

signature.asc
Description: OpenPGP digital signature