On 06/07/2016 10:20 AM, Daniel P. Berrange wrote: > On Tue, Jun 07, 2016 at 10:09:48AM -0600, Eric Blake wrote: >> On 06/07/2016 04:11 AM, Daniel P. Berrange wrote: >>> The current approach for pretty-printing QAPI types is to >>> convert them to JSON using the QMP output visitor and then >>> pretty-print the JSON document. This has an unfixable problem >>> that structs get their keys printed out in random order, since >>> JSON dicts do not contain any key ordering information. >>> >>> To address this, introduce a text output visitor that can >>> directly pretty print a QAPI type into a string. >>> >>> Signed-off-by: Daniel P. Berrange <[email protected]> >>> --- >>> include/qapi/text-output-visitor.h | 73 ++++++++++++ >>> include/qapi/visitor-impl.h | 5 +- >>> include/qapi/visitor.h | 5 +- >>> qapi/Makefile.objs | 1 + >>> qapi/opts-visitor.c | 5 +- >>> qapi/qapi-dealloc-visitor.c | 4 +- >>> qapi/qapi-visit-core.c | 9 +- >>> qapi/qmp-input-visitor.c | 5 +- >>> qapi/qmp-output-visitor.c | 4 +- >>> qapi/string-input-visitor.c | 5 +- >>> qapi/string-output-visitor.c | 5 +- >> >> Why can't we enhance the existing string-output-visitor to handle structs? > > string-output-visitor seems to be doing something very > different from this. In particular it only ever seems > to output the values, never the field names. So if we > did enhance string-output-visitor, we'd basically have > to make all of its code conditional to output in one > style or the other style, at which point I didn't think > it was really buying us anything vs a new visitor.
That is, it was always doing a top-level visit of a scalar or array of scalars, and nothing else. It may still be something that can be merged. Maybe I should take a rough shot at it, since I have ideas on how to use a common handler for name/list index (and do nothing at the top level), then the rest of each callback is independent from what name prefix, if any, was output. On the other hand, I guess the way intList is handled (compacting it into a single list, instead of each element of the list), may indeed be a reason to keep it as two visitors. >> Why do we need to list the element size twice? Or is one the size of >> the GenericList object wrapping the element? I'm still not convinced we >> need the size of an element at this point in the visit. > > 'size_t element' isn't the size of an element, it is the really > the list index - eg 1, 2, 3, 4, etc. I didn't call it 'index' > because that causes a clashing symbol, but I guess I could > have used 'idx' instead. The perils of replying to email text in the order I read it. I think I figured that out later on. And it is indeed annoying that older gcc warns about conflicts with shadowing 'index'. >> Don't we already have a util/ function for pretty-printing a size? In >> fact, doesn't the existing StringOutputVisitor have code for doing it? > > Guess where this code was copied from - StringOutputVisitor :-) If this > idea is amenable in general, I'll go back and cleanup this patch be better. A common helper may be nice, if we have two visitors both wanting to use it. >> >> Oooooh, I see. You're using 'element' as the index within the array, >> not as a size. I think naming it 'idx' (or 'index', if that doesn't >> cause older compilers to barf for shadowing a function name), would make >> more sense, but it definitely highlights the need for documentation. >> Also, why does element 0 have to be special-cased? > > The pattern of calling means 'next_list' is invoked /after/ > each element is visited, but we want to print out the header > /before/ each element. So I had to special case 0, and also > use the 'if(ret)' check to skip printing a header after the > last element. But if we have a common helper function (see json_output_name() in my JSON series), then that helper will be called before every element, and can easily tell whether we are in struct context (name is non-null, print it) or list context (name is NULL, print our current counter then increment it), rather than having to special case the code around the elements. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
