On 11/12/2015 08:46 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes: > >> Similar to the previous commit, move the detection of a collision >> in enum values from parse time to QAPISchemaEnumType.check(). >> This happens to also detect collisions in union branch names >> mapping to the same enum value, even when the names do not >> collide case-wise. So for a decent error message, we have to >> determine if the enum is implicit (and if so where the real >> collision lies). >>
>> @@ -873,8 +856,30 @@ class QAPISchemaEnumType(QAPISchemaType): >> self.values = values >> self.prefix = prefix >> >> + def _describe(self, schema): >> + # If the enum is implicit, report the error on behalf of >> + # the union or alternate that triggered the enum >> + if self.is_implicit(): >> + owner = schema.lookup_type(self.name[:-4]) >> + assert owner >> + if isinstance(owner, QAPISchemaAlternateType): >> + return "Alternate '%s' branch" % owner.name > > Didn't we just drop this kind of implicit enum? D'oh; rebase churn reordering patches. > >> + else: >> + return "Union '%s' branch" % owner.name >> + else: >> + return "Enum '%s' value" % self.name > > I like to call it "member" rather than value, because it avoids > confusion with the numeric value of the C enumeration constant generated > for it. Sure, that sounds slightly better. > > The conditional isn't exactly elegant, but it would do. I'm not 100% > convinced we need it, though. self.info already points to whatever > defined this enum, either an explicit enum definition, or a simple union > definition. How do the error messages come out if we dumb down to > "Member '%s'"? > > A method with a similar purpose exists in QAPISchemaObjectTypeMember, > but it's spelled describe(). It's used only from within the class. Are you talking about _pretty_owner()? Or the actual describe() that calls _pretty_owner()? > Rename it to match this one? > >> + >> def check(self, schema): >> - assert len(set(self.values)) == len(self.values) >> + # Check for collisions on the generated C enum values >> + seen = {c_enum_const(self.name, 'MAX'): '(automatic MAX)'} >> + for value in self.values: >> + c_value = c_enum_const(self.name, value) >> + if c_value in seen: >> + raise QAPIExprError(self.info, >> + "%s '%s' clashes with '%s'" >> + % (self._describe(schema), value, >> + seen[c_value])) >> + seen[c_value] = value > > Loop body is very similar to QAPISchemaObjectTypeMember.check_clash(). > Differences: > > * c_enum_const(enum_name, member_name) vs. c_name(member_name).upper() > > This isn't really a difference, because the former returns the latter > prefixed by a string that doesn't vary in the loop. Well, it _is_ a difference if c_name() munges differently than c_enum_const() (the whole question of whether 'OneTwo' and 'One-Two' are detected as clashes); but then again, I have the patches that try to rework c_enum_const() to use just c_name().upper(). > > One could argue that using c_enum_const() lets us abstract from what > it does, but that's an illusion. We rely on it when we generate union > members called c_name(member_name) without checking for collisions > again. > > By the way, c_enum_const(self.name, value, self.prefix) would be more > correct. Doesn't matter here, of course. > > Therefore, I'd be very much tempted to use c_name(member_name).upper() > here as well. > > * The error message. But I suspect the same "Member '%s' clashes with > '%s'" could do for both. > > If I'm right and we can drop the differences, the common code could > become a helper function. I'll play with it and see what pops out. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature