Eric Blake <ebl...@redhat.com> writes: > On 07/07/2016 04:52 AM, Markus Armbruster wrote: >> Eric Blake <ebl...@redhat.com> writes: >> >>> Turn on the ability to pass command and event arguments in >>> a single boxed parameter, which must name a non-empty type >>> (although the type can be a struct with all optional members). >>> For structs, it makes it possible to pass a single qapi type >>> instead of a breakout of all struct members (useful if the >>> arguments are already in a struct or if the number of members >>> is large); for other complex types, it is now possible to use >>> a union or alternate as the data for a command or event. >>> >>> The empty type may be technically feasible if needed down the >>> road, but it's easier to forbid it now and relax things to allow >>> it later, than it is to allow it now and have to special case >>> how the generated 'q_empty' type is handled (see commit 7ce106a9 >>> for reasons why nothing is generated for the empty type). An >>> alternate type is never considered empty. >>> >>> Generated code is unchanged, as long as no client uses the >>> new feature. >>> >>> Signed-off-by: Eric Blake <ebl...@redhat.com> >>> > >>> @@ -1180,9 +1195,18 @@ class QAPISchemaCommand(QAPISchemaEntity): >>> def check(self, schema): >>> if self._arg_type_name: >>> self.arg_type = schema.lookup_type(self._arg_type_name) >>> - assert isinstance(self.arg_type, QAPISchemaObjectType) >>> - assert not self.arg_type.variants # not implemented >>> - assert not self.box # not implemented >>> + assert (isinstance(self.arg_type, QAPISchemaObjectType) or >>> + isinstance(self.arg_type, QAPISchemaAlternateType)) >>> + self.arg_type.check(schema) >> >> qapi.py recurses .check() only when necessary, because undisciplined >> recursion can easily become cyclic. >> >> I think you need self.arg_type.check() here so you can >> self.arg_type.is_empty() below. Correct? > > Correct. And should not be unbounded - a command depends on a type, but > no type depends on a command, so this does not introduce new recursion.
Yes. I like to avoid .check() recursion whenever practical, to keep the termination argument simple, but since I can't see how to avoid it here, we'll go with your code. >>> + if self.box: >>> + if self.arg_type.is_empty(): >>> + raise QAPIExprError(self.info, >>> + "Cannot use 'box' with empty type") >>> + else: >>> + assert not self.arg_type.variants >> >> Lost: assert isinstance(self.arg_type, QAPISchemaObjectType), or the >> equivalent assert not isinstance(self.arg_type, QAPISchemaAlternateType). > > Or rather, implicitly hidden. Only QAPISchemaObjectType and > QAPISchemaAlternateType have a .is_empty() or .variants, so if any other > type is passed, python will complain about a missing attribute (which is > just as effective as the assert() used to be). You're right. >>> + elif self.box: >>> + raise QAPIExprError(self.info, >>> + "Use of 'box' requires 'data'") >>> if self._ret_type_name: >>> self.ret_type = schema.lookup_type(self._ret_type_name) >>> assert isinstance(self.ret_type, QAPISchemaType) >>> @@ -1204,9 +1228,18 @@ class QAPISchemaEvent(QAPISchemaEntity): >>> def check(self, schema): >>> if self._arg_type_name: >>> self.arg_type = schema.lookup_type(self._arg_type_name) >>> - assert isinstance(self.arg_type, QAPISchemaObjectType) >>> - assert not self.arg_type.variants # not implemented >>> - assert not self.box # not implemented >>> + assert (isinstance(self.arg_type, QAPISchemaObjectType) or >>> + isinstance(self.arg_type, QAPISchemaAlternateType)) >>> + self.arg_type.check(schema) >>> + if self.box: >>> + if self.arg_type.is_empty(): >>> + raise QAPIExprError(self.info, >>> + "Cannot use 'box' with empty type") >>> + else: >>> + assert not self.arg_type.variants >> >> Likewise. > > And same argument about being implicitly correct. I can either document > this in the commit message (to call it out as intentional) or restore > the asserts; up to you which is cleaner. Let's restore the assertions, because they do double-duty documenting intent here. >>> == Client JSON Protocol introspection == >>> >> [Tests snipped, they look good...] >> >> Having read PATCH 07+08 another time, I got one more stylistic remark: >> I'd name the flag @boxed, not @box. It's not a box, it's a flag that >> tells us that whatever it applies to is boxed. > > Sounds reasonable, so looks like a v9 is worth posting. Yes, but it should be few and simple changes, thus quick to review.
