| Message ID | ed5d831b-2035-74b5-b12a-01f443c8a896@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > On 06/29/2018 02:55 PM, Eric Blake wrote: >> We consciously chose in commit 1a9a507b to hide QAPI type names >> from the generated introspection output, but added a command line >> option -u to unmask the type name when doing a debug build. At >> that time, we generated a monolithic C string, so there was no >> better way to do things (we could not really inject comments). >> >> Later, in commit 7d0f982b, we switched the generation to output >> a QLit object, in part to make it easier for future addition of >> conditional compilation. But this switch has also made it easier >> to interject strategic comments. That commit also forgot to >> delete some now-stale comments about long generated line lengths. >> >> For now, type name debug aid comments are only output once per >> meta-type, rather than at all uses of the number used to encode >> the type to the introspection data. But this is still a lot >> more convenient than having to regenerate the file with the >> unmask operation temporarily turned on. > ... > >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> scripts/qapi/introspect.py | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) > > Misses a change to docs/devel/qapi-code.gen.tx: > > diff --git i/docs/devel/qapi-code-gen.txt w/docs/devel/qapi-code-gen.txt > index 88a70e4d451..0c98c7d7b6c 100644 > --- i/docs/devel/qapi-code-gen.txt > +++ w/docs/devel/qapi-code-gen.txt > @@ -1392,6 +1392,7 @@ Example: > { } > })), > QLIT_QDICT(((QLitDictEntry[]) { > + /* q_empty */ > { "members", QLIT_QLIST(((QLitObject[]) { > { } > })) }, Yes. >> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py >> index 6ad198ae5b5..b37160292bc 100644 >> --- a/scripts/qapi/introspect.py >> +++ b/scripts/qapi/introspect.py >> @@ -35,10 +35,14 @@ def to_qlit(obj, level=0, suppress_first_indent=False): >> elif isinstance(obj, dict): >> elts = [] >> for key, value in sorted(obj.items()): >> + if key == 'comment': >> + continue >> elts.append(indent(level + 1) + '{ %s, %s }' % >> (to_c_string(key), to_qlit(value, level + 1, True))) >> elts.append(indent(level + 1) + '{}') >> ret += 'QLIT_QDICT(((QLitDictEntry[]) {\n' >> + if obj.get('comment'): >> + ret += indent(level + 1) + '/* %s */\n' % obj['comment'] >> ret += ',\n'.join(elts) + '\n' >> ret += indent(level) + '}))' >> elif isinstance(obj, bool): to_qlit() converts from a natural representation of JSON in Python to JSON text. Your patch breaks it for JSON objects that have a member named 'comment'. Related work: Marc-André's "[PATCH v6 09/15] qapi-introspect: add preprocessor conditions to generated QLit" uses tuples to augment JSON. He uses them for conditionals, but the technique could be generalized, say from his (jobj, ifcond) to (jobj, { 'if': ifcond, '#': comment }. >> @@ -78,7 +82,6 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor): >> for typ in self._used_types: >> typ.visit(self) >> # generate C >> - # TODO can generate awfully long lines >> name = c_name(self._prefix, protect=False) + 'qmp_schema_qlit' >> self._genh.add(mcgen(''' >> #include "qapi/qmp/qlit.h" >> @@ -118,8 +121,8 @@ const QLitObject %(c_name)s = %(c_string)s; >> if typ not in self._used_types: >> self._used_types.append(typ) >> # Clients should examine commands and events, not types. Hide >> - # type names to reduce the temptation. Also saves a few >> - # characters. >> + # type names as integers to reduce the temptation, but provide >> + # comments for debugging aid. > > When I wrote the patch, I assumed the final sentence of the comment > was talking about fewer characters in the generated .c file (I nuked > it, since adding comments makes for a longer, not shorter, generated > .c file); but now that I'm writing this email, I see it could also > refer to fewer characters sent over the wire in the QMP command (which Yes, that was my intent. > length is unchanged by this patch, but was a 13KiB savings compared to > the unmasked version, per commit 1a9a507b). So feel free to bikeshed > this comment. I think I'd merely clarify the comment's last sentence by adding "on the wire", ... >> if isinstance(typ, QAPISchemaBuiltinType): >> return typ.name >> if isinstance(typ, QAPISchemaArrayType): >> @@ -128,6 +131,8 @@ const QLitObject %(c_name)s = %(c_string)s; >> >> def _gen_qlit(self, name, mtype, obj): >> if mtype not in ('command', 'event', 'builtin', 'array'): >> + if not self._unmask: ... and perhaps briefly explain here why we generate a comment. >> + obj['comment'] = name >> name = self._name(name) >> obj['name'] = name >> obj['meta-type'] = mtype
On 07/02/2018 01:43 PM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 06/29/2018 02:55 PM, Eric Blake wrote: >>> We consciously chose in commit 1a9a507b to hide QAPI type names >>> from the generated introspection output, but added a command line >>> option -u to unmask the type name when doing a debug build. At >>> that time, we generated a monolithic C string, so there was no >>> better way to do things (we could not really inject comments). >>> >>> Later, in commit 7d0f982b, we switched the generation to output >>> a QLit object, in part to make it easier for future addition of >>> conditional compilation. But this switch has also made it easier >>> to interject strategic comments. That commit also forgot to >>> delete some now-stale comments about long generated line lengths. >>> >>> For now, type name debug aid comments are only output once per >>> meta-type, rather than at all uses of the number used to encode >>> the type to the introspection data. But this is still a lot >>> more convenient than having to regenerate the file with the >>> unmask operation temporarily turned on. >> ... > to_qlit() converts from a natural representation of JSON in Python to > JSON text. > > Your patch breaks it for JSON objects that have a member named > 'comment'. Hmm. None do, presently. And I don't see where it would be permitted in the QAPI schema for query-qmp-schema. > > Related work: Marc-André's "[PATCH v6 09/15] qapi-introspect: add > preprocessor conditions to generated QLit" uses tuples to augment JSON. > He uses them for conditionals, but the technique could be generalized, > say from his (jobj, ifcond) to (jobj, { 'if': ifcond, '#': comment }. Yeah, I can probably rebase on top of that (and at this point, it's late enough to probably have missed 3.0 anyways, so I'm not time-crunched for getting it done right now).
Eric Blake <eblake@redhat.com> writes: > On 07/02/2018 01:43 PM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> On 06/29/2018 02:55 PM, Eric Blake wrote: >>>> We consciously chose in commit 1a9a507b to hide QAPI type names >>>> from the generated introspection output, but added a command line >>>> option -u to unmask the type name when doing a debug build. At >>>> that time, we generated a monolithic C string, so there was no >>>> better way to do things (we could not really inject comments). >>>> >>>> Later, in commit 7d0f982b, we switched the generation to output >>>> a QLit object, in part to make it easier for future addition of >>>> conditional compilation. But this switch has also made it easier >>>> to interject strategic comments. That commit also forgot to >>>> delete some now-stale comments about long generated line lengths. >>>> >>>> For now, type name debug aid comments are only output once per >>>> meta-type, rather than at all uses of the number used to encode >>>> the type to the introspection data. But this is still a lot >>>> more convenient than having to regenerate the file with the >>>> unmask operation temporarily turned on. >>> ... > >> to_qlit() converts from a natural representation of JSON in Python to >> JSON text. >> >> Your patch breaks it for JSON objects that have a member named >> 'comment'. > > Hmm. None do, presently. And I don't see where it would be permitted > in the QAPI schema for query-qmp-schema. So far, to_qlit() is completely oblivious of the schema; it just crunches JSON. I'd like to keep it that way, if practical. >> Related work: Marc-André's "[PATCH v6 09/15] qapi-introspect: add >> preprocessor conditions to generated QLit" uses tuples to augment JSON. >> He uses them for conditionals, but the technique could be generalized, >> say from his (jobj, ifcond) to (jobj, { 'if': ifcond, '#': comment }. > > Yeah, I can probably rebase on top of that (and at this point, it's > late enough to probably have missed 3.0 anyways, so I'm not > time-crunched for getting it done right now). It's a lovely improvement for developers, but it'll be just as lovely in 3.1 :)
diff --git i/docs/devel/qapi-code-gen.txt w/docs/devel/qapi-code-gen.txt index 88a70e4d451..0c98c7d7b6c 100644 --- i/docs/devel/qapi-code-gen.txt +++ w/docs/devel/qapi-code-gen.txt @@ -1392,6 +1392,7 @@ Example: { } })), QLIT_QDICT(((QLitDictEntry[]) { + /* q_empty */ { "members", QLIT_QLIST(((QLitObject[]) { { } })) },