Eric Blake <ebl...@redhat.com> writes:
> On 10/02/2015 07:19 AM, Markus Armbruster wrote:
>> Eric Blake <ebl...@redhat.com> 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. Doing this was made
>>> easier by adding a member.c_name() helper function.
>>
>> This gains protection against colliding C names. It keeps protection
>> against colliding QMP names as long as QMP name collision implies C name
>> collision. I think that's the case, but it's definitely worth spelling
>> out in the code, and possibly in the commit message.
>>
>>> As this is the first error raised within the QAPISchema*.check()
>>> methods, we also have to pass 'info' through the call stack, and
>>> fix the overall 'try' to display errors detected during the
>>> check() phase.
>>
>> Could also be a separate patch, if the parts are easier to review.
>
> I'm still debating about that.
>
>
>>> + def check(self, schema, info, all_members, seen):
>>> + if self.c_name() in seen:
>>> + raise QAPIExprError(info,
>>> + "%s collides with %s"
>>> + % (self.describe(),
>>> + seen[self.c_name()].describe()))
>>> self.type = schema.lookup_type(self._type_name)
>>> assert self.type
>>> all_members.append(self)
>>> - seen[self.name] = self
>>> + seen[self.c_name()] = self
>>> +
>>> + def c_name(self):
>>> + return c_name(self.name)
>>
>> Why wrap function c_name() in a method? Why not simply call the
>> function?
>
> 'self.c_name()' is shorter than 'c_name(self.name)'. And I already had
> long lines with that seen[self.c_name()].describe() pattern.
You could also try a local variable: cnam = c_name(self.name).
>> It's method in QAPISchemaEntity only because this lets us add special
>> cases in a neat way.
>
> True, but I _did_ mention in the commit message that I did it for less
> typing.
>
> But as to special cases, yes, I have one in mind (although I have not
> played with it yet). In v5 19/46 Simplify visiting of alternate types,
> I want to change alternates to have variants.tag_member == None, and the
> generated C code to use 'qtype_code type;' as the tag variable. In the
> v5 representation, it led to a lot of special-casing (many uses of
> QAPISchemaObjectTypeVariants became more complex because tag_member was
> not always defined). But now that I've been working on these front-end
> patches, my idea was to do something like:
>
> class QAPISchemaVariantTag(QAPISchemaObjectTypeMember):
> def __init__(self, info):
> QAPISchemaObjectTypeMember(self, 'type', None, False)
>
> def c_type(self):
> return 'qtype_code'
>
> and then, we WOULD need member.c_type() rather than member.type.c_type()
> (at which point having member.c_name() to match member.c_type() makes
> more sense).
I'm afraid I don't have enough context to grok this late on Friday :)
>>> @@ -1028,23 +1037,24 @@ class QAPISchemaObjectTypeVariants(object):
>>> self.tag_member = tag_member
>>> self.variants = variants
>>>
>>> - def check(self, schema, members, seen):
>>> + def check(self, schema, info, members, seen):
>>> if self.tag_name:
>>> - self.tag_member = seen[self.tag_name]
>>> + self.tag_member = seen[c_name(self.tag_name)]
>>> + assert self.tag_name == self.tag_member.name
>>
>> Assertion should hold before the patch, but it's kind of trivial then.
>
> My worry here was that if I have:
>
> { 'enum': 'Enum', 'data': ['a'] }
> { 'base': 'Base', 'data': { 'b-c': 'Enum' } }
> { 'union': 'Flat', 'base': 'Base', 'discriminator': 'b_c',
> 'data': { 'a': 'Struct...' } }
>
> the old ad hoc parser rejects 'b_c' as not being a member of Enum (bad
> discriminator). But once we convert from the old parser to the check()
> method (basically, anywhere the check() methods now have an assert will
> become if statements that raise an exception), we need to make sure that
> we don't get confused by the fact that seen[c_name('b_c')] maps to the
> same value as seen[c_name('b-c')], so that we are still flagging the
> flat union as invalid.
I had already flagged a few assertions as "holds before the patch" when
I got here, and felt an urge to flag this one, too, for completeness. I
don't object to you adding the assertion.
>> I'm fine with not splitting this patch. I'd be also fine with splitting
>> it up. Your choice.
>
> I'm still thinking about it; may depend on how much other refactoring I do.
