| Message ID | 1456443528-13901-12-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > And use it in qapi-types and qapi-event. Down the road, we may > want to lift our artificial restriction of no variants at the > top level of an event, at which point, inlining our check for > whether members is empty will no longer be sufficient, but > adding a check for variants adds verbosity; in the meantime, > add some asserts in places where we don't handle variants. Perhaps I'm just running out of steam for today, but I've read this twice, and still don't get why adding these assertions goes in the same patch as adding the helper, or what it has to do with events. > More immediately, the new .is_empty() helper will help fix a bug > in qapi-visit in the next patch, where the generator did not > handle an explicit empty type in the same was as a missing type. same way > > No change to generated code. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v2: no change > v1: add some asserts > Previously posted as part of qapi cleanup subset E: > v9: improve commit message > v8: no change > v7: rebase to context change > v6: new patch > --- > scripts/qapi.py | 5 +++++ > scripts/qapi-event.py | 7 ++++--- > scripts/qapi-types.py | 2 +- > 3 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 6c52fe5..83080b3 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -962,6 +962,7 @@ class QAPISchemaObjectType(QAPISchemaType): > assert isinstance(self.base, QAPISchemaObjectType) > self.base.check(schema) > self.base.check_clash(schema, self.info, seen) > + assert not self.base.variants > for m in self.local_members: > m.check(schema) > m.check_clash(self.info, seen) This is the "some asserts" ;) > @@ -983,6 +984,10 @@ class QAPISchemaObjectType(QAPISchemaType): > # See QAPISchema._make_implicit_object_type() > return self.name[0] == ':' > > + def is_empty(self): > + assert self.members is not None > + return not self.members and not self.variants > + > def c_name(self): > assert not self.is_implicit() > return QAPISchemaType.c_name(self) > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py > index fb579dd..808ed80 100644 > --- a/scripts/qapi-event.py > +++ b/scripts/qapi-event.py > @@ -39,7 +39,7 @@ def gen_event_send(name, arg_type): > ''', > proto=gen_event_send_proto(name, arg_type)) > > - if arg_type and arg_type.members: > + if arg_type and not arg_type.is_empty(): > ret += mcgen(''' > QmpOutputVisitor *qov; > Visitor *v; Oh, you don't just add a helper, you actually *change* the condition! Perhaps the commit message would be easier to understand if it explained that first. > @@ -58,7 +58,8 @@ def gen_event_send(name, arg_type): > ''', > name=name) > > - if arg_type and arg_type.members: > + if arg_type and not arg_type.is_empty(): > + assert not arg_type.variants > ret += mcgen(''' > qov = qmp_output_visitor_new(); > v = qmp_output_get_visitor(qov); > @@ -88,7 +89,7 @@ out_obj: > ''', > c_enum=c_enum_const(event_enum_name, name)) > > - if arg_type and arg_type.members: > + if arg_type and not arg_type.is_empty(): > ret += mcgen(''' > out: > qmp_output_visitor_cleanup(qov); > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index 0306a88..6c1923d 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -90,7 +90,7 @@ struct %(c_name)s { > # potential issues with attempting to malloc space for zero-length > # structs in C, and also incompatibility with C++ (where an empty > # struct is size 1). > - if not (base and base.members) and not members and not variants: > + if (not base or base.is_empty()) and not members and not variants: > ret += mcgen(''' > char qapi_dummy_for_empty_struct; > ''') I figure the case for the helper based on this patch alone is making the code a bit more future-proof. Suggest you try to explain that in your commit message, including against what future change exactly you're proofing the code. Haven't reviewed for completeness.
On 03/02/2016 12:04 PM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> And use it in qapi-types and qapi-event. Down the road, we may >> want to lift our artificial restriction of no variants at the >> top level of an event, at which point, inlining our check for >> whether members is empty will no longer be sufficient, but >> adding a check for variants adds verbosity; in the meantime, >> add some asserts in places where we don't handle variants. > > Perhaps I'm just running out of steam for today, but I've read this > twice, and still don't get why adding these assertions goes in the same > patch as adding the helper, or what it has to do with events. And yet it was the review on the earlier posting that caused me to add asserts; maybe re-reading that thread will help refresh memory, and spur an idea for how to better express it in the commit message: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04726.html > >> More immediately, the new .is_empty() helper will help fix a bug >> in qapi-visit in the next patch, where the generator did not >> handle an explicit empty type in the same was as a missing type. > > same way [Ever wonder if I intentionally stick in a typo, just to see who will notice? Or maybe it really was a slip of the finger...] >> +++ b/scripts/qapi-event.py >> @@ -39,7 +39,7 @@ def gen_event_send(name, arg_type): >> ''', >> proto=gen_event_send_proto(name, arg_type)) >> >> - if arg_type and arg_type.members: >> + if arg_type and not arg_type.is_empty(): >> ret += mcgen(''' >> QmpOutputVisitor *qov; >> Visitor *v; > > Oh, you don't just add a helper, you actually *change* the condition! > Perhaps the commit message would be easier to understand if it explained > that first. The old condition: arg_type and arg_type.members New condition: arg_type and (arg_type.members or arg_type.variants) But we know there are no variants, since unions cannot (yet) be passed as event 'data', so the condition is the same effect now, and future-proofing for a future patch when I do allow unions in events. >> +++ b/scripts/qapi-types.py >> @@ -90,7 +90,7 @@ struct %(c_name)s { >> # potential issues with attempting to malloc space for zero-length >> # structs in C, and also incompatibility with C++ (where an empty >> # struct is size 1). >> - if not (base and base.members) and not members and not variants: >> + if (not base or base.is_empty()) and not members and not variants: >> ret += mcgen(''' >> char qapi_dummy_for_empty_struct; >> ''') > > I figure the case for the helper based on this patch alone is making the > code a bit more future-proof. Suggest you try to explain that in your > commit message, including against what future change exactly you're > proofing the code. And here, bases cannot (yet) have variants, but that's also on my plate of things I'd like to support in the future. > > Haven't reviewed for completeness. >
On 03/02/2016 12:04 PM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> And use it in qapi-types and qapi-event. Down the road, we may >> want to lift our artificial restriction of no variants at the >> top level of an event, at which point, inlining our check for >> whether members is empty will no longer be sufficient, but >> adding a check for variants adds verbosity; in the meantime, >> add some asserts in places where we don't handle variants. > > Perhaps I'm just running out of steam for today, but I've read this > twice, and still don't get why adding these assertions goes in the same > patch as adding the helper, or what it has to do with events. Okay, will split this into two patches for v3.
Eric Blake <eblake@redhat.com> writes: > On 03/02/2016 12:04 PM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> And use it in qapi-types and qapi-event. Down the road, we may >>> want to lift our artificial restriction of no variants at the >>> top level of an event, at which point, inlining our check for >>> whether members is empty will no longer be sufficient, but >>> adding a check for variants adds verbosity; in the meantime, >>> add some asserts in places where we don't handle variants. >> >> Perhaps I'm just running out of steam for today, but I've read this >> twice, and still don't get why adding these assertions goes in the same >> patch as adding the helper, or what it has to do with events. > > And yet it was the review on the earlier posting that caused me to add > asserts; maybe re-reading that thread will help refresh memory, and spur > an idea for how to better express it in the commit message: > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04726.html I suspect what's needed is a clearer commit message a fresher mind reading it. >>> More immediately, the new .is_empty() helper will help fix a bug >>> in qapi-visit in the next patch, where the generator did not >>> handle an explicit empty type in the same was as a missing type. >> >> same way > > [Ever wonder if I intentionally stick in a typo, just to see who will > notice? Or maybe it really was a slip of the finger...] > >>> +++ b/scripts/qapi-event.py >>> @@ -39,7 +39,7 @@ def gen_event_send(name, arg_type): >>> ''', >>> proto=gen_event_send_proto(name, arg_type)) >>> >>> - if arg_type and arg_type.members: >>> + if arg_type and not arg_type.is_empty(): >>> ret += mcgen(''' >>> QmpOutputVisitor *qov; >>> Visitor *v; >> >> Oh, you don't just add a helper, you actually *change* the condition! >> Perhaps the commit message would be easier to understand if it explained >> that first. > > The old condition: > arg_type and arg_type.members > > New condition: > arg_type and (arg_type.members or arg_type.variants) > > But we know there are no variants, since unions cannot (yet) be passed > as event 'data', so the condition is the same effect now, and > future-proofing for a future patch when I do allow unions in events. Unless allowing unions makes generators nicer, I'll want to see a compelling use case. Can you give me an idea what .is_empty() does for *this* patch series? Wait, you did: "will help fix a bug [...] in the next patch". Perhaps a slight rearrangement would make things easier to grok: start with the bug fix, and introduce .is_empty() there. In the next patch, say "In these places, the intent is to test for empty, but the actual code exploits that there can be no variants. Using .is_empty() instead is a bit clearer and a bit more robust." >>> +++ b/scripts/qapi-types.py >>> @@ -90,7 +90,7 @@ struct %(c_name)s { >>> # potential issues with attempting to malloc space for zero-length >>> # structs in C, and also incompatibility with C++ (where an empty >>> # struct is size 1). >>> - if not (base and base.members) and not members and not variants: >>> + if (not base or base.is_empty()) and not members and not variants: >>> ret += mcgen(''' >>> char qapi_dummy_for_empty_struct; >>> ''') >> >> I figure the case for the helper based on this patch alone is making the >> code a bit more future-proof. Suggest you try to explain that in your >> commit message, including against what future change exactly you're >> proofing the code. > > And here, bases cannot (yet) have variants, but that's also on my plate > of things I'd like to support in the future. Also needs a use case. >> Haven't reviewed for completeness.
Eric Blake <eblake@redhat.com> writes: > On 03/02/2016 12:04 PM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> And use it in qapi-types and qapi-event. Down the road, we may >>> want to lift our artificial restriction of no variants at the >>> top level of an event, at which point, inlining our check for >>> whether members is empty will no longer be sufficient, but >>> adding a check for variants adds verbosity; in the meantime, >>> add some asserts in places where we don't handle variants. >> >> Perhaps I'm just running out of steam for today, but I've read this >> twice, and still don't get why adding these assertions goes in the same >> patch as adding the helper, or what it has to do with events. > > Okay, will split this into two patches for v3. A better commit message might do. Use your judgement.
diff --git a/scripts/qapi.py b/scripts/qapi.py index 6c52fe5..83080b3 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -962,6 +962,7 @@ class QAPISchemaObjectType(QAPISchemaType): assert isinstance(self.base, QAPISchemaObjectType) self.base.check(schema) self.base.check_clash(schema, self.info, seen) + assert not self.base.variants for m in self.local_members: m.check(schema) m.check_clash(self.info, seen) @@ -983,6 +984,10 @@ class QAPISchemaObjectType(QAPISchemaType): # See QAPISchema._make_implicit_object_type() return self.name[0] == ':' + def is_empty(self): + assert self.members is not None + return not self.members and not self.variants + def c_name(self): assert not self.is_implicit() return QAPISchemaType.c_name(self) diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py index fb579dd..808ed80 100644 --- a/scripts/qapi-event.py +++ b/scripts/qapi-event.py @@ -39,7 +39,7 @@ def gen_event_send(name, arg_type): ''', proto=gen_event_send_proto(name, arg_type)) - if arg_type and arg_type.members: + if arg_type and not arg_type.is_empty(): ret += mcgen(''' QmpOutputVisitor *qov; Visitor *v; @@ -58,7 +58,8 @@ def gen_event_send(name, arg_type): ''', name=name) - if arg_type and arg_type.members: + if arg_type and not arg_type.is_empty(): + assert not arg_type.variants ret += mcgen(''' qov = qmp_output_visitor_new(); v = qmp_output_get_visitor(qov); @@ -88,7 +89,7 @@ out_obj: ''', c_enum=c_enum_const(event_enum_name, name)) - if arg_type and arg_type.members: + if arg_type and not arg_type.is_empty(): ret += mcgen(''' out: qmp_output_visitor_cleanup(qov); diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 0306a88..6c1923d 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -90,7 +90,7 @@ struct %(c_name)s { # potential issues with attempting to malloc space for zero-length # structs in C, and also incompatibility with C++ (where an empty # struct is size 1). - if not (base and base.members) and not members and not variants: + if (not base or base.is_empty()) and not members and not variants: ret += mcgen(''' char qapi_dummy_for_empty_struct; ''')
And use it in qapi-types and qapi-event. Down the road, we may want to lift our artificial restriction of no variants at the top level of an event, at which point, inlining our check for whether members is empty will no longer be sufficient, but adding a check for variants adds verbosity; in the meantime, add some asserts in places where we don't handle variants. More immediately, the new .is_empty() helper will help fix a bug in qapi-visit in the next patch, where the generator did not handle an explicit empty type in the same was as a missing type. No change to generated code. Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: no change v1: add some asserts Previously posted as part of qapi cleanup subset E: v9: improve commit message v8: no change v7: rebase to context change v6: new patch --- scripts/qapi.py | 5 +++++ scripts/qapi-event.py | 7 ++++--- scripts/qapi-types.py | 2 +- 3 files changed, 10 insertions(+), 4 deletions(-)