>> Would it be valid to do something like this (skipping elements without a >> proper visit_type_int) >> >> visit_start_list(); >> visit_next_list(); more input, returns "there's more" >> visit_next_list(); parses "1-3,", buffers 2-3, skips over 1 >> visit_type_int(); returns 2 >> ... > > Excellent question! > > Here's the relevant part of visit_start_list()'s contract in visitor.h: > > * After visit_start_list() succeeds, the caller may visit its members > * one after the other. A real visit (where @obj is non-NULL) uses > * visit_next_list() for traversing the linked list, while a virtual > * visit (where @obj is NULL) uses other means. For each list > * element, call the appropriate visit_type_FOO() with name set to > * NULL and obj set to the address of the value member of the list > * element. Finally, visit_end_list() needs to be called with the > * same @list to clean up, even if intermediate visits fail. See the > * examples above. > > So, you *may* visit members, and you *must* call visit_end_list(). > > But what are "real" and "virtual" visits? Again, the contract: > > * @list must be non-NULL for a real walk, in which case @size > * determines how much memory an input or clone visitor will allocate > * into *@list (at least sizeof(GenericList)). Some visitors also > * allow @list to be NULL for a virtual walk, in which case @size is > * ignored. > > I'm not sure whether I just decreased or increased global confusion ;)
I was reading over these comments and got slightly confused :) > > The file comment explains: > > * The QAPI schema defines both a set of C data types, and a QMP wire > * format. QAPI objects can contain references to other QAPI objects, > * resulting in a directed acyclic graph. QAPI also generates visitor > * functions to walk these graphs. This file represents the interface > * for doing work at each node of a QAPI graph; it can also be used > * for a virtual walk, where there is no actual QAPI C struct. > > A real walk with an output visitor works like this (error handling > omitted for now): > > Error *err = NULL; > intList *tail; > size_t size = sizeof(**obj); > > // fetch list's head into *obj, start the list in output > visit_start_list(v, name, (GenericList **)obj, size, &err); > > // iterate over list's tails > // below the hood, visit_next_list() iterates over the GenericList > for (tail = *obj; tail; > tail = (intList *)visit_next_list(v, (GenericList *)tail, size)) { > // visit current tail's first member, add it to output > visit_type_int(v, NULL, &tail->value, &err); > } > > // end the list in output > visit_end_list(v, (void **)obj); > > The exact same code works for a real walk with an input visitor, where > visit_next_list() iterates over the *input* and builds up the > GenericList. Compare qobject_input_next_list() and > qobject_output_next_list(). > > The code above is a straight copy of generated visit_type_intList() with > error handling cut and comments added. > > A real walk works on a QAPI-generated C type. QAPI generates a real > walk for each such type. Open-coding a real walk would senselessly > duplicate the generated one, so we don't. Thus, a real walk always > visits each member. > > Okay, I lied: it visits each member until it runs into an error and > bails out. See generated visit_type_intList() in > qapi/qapi-builtin-visit.c. > > A virtual walk doesn't work with a GenericList *. Calling > visit_next_list() makes no sense then. visitor.h gives this example of > a virtual walk: Alright, so we must not support virtual walks. But supporting it would not harm :) > > * Thus, a virtual walk corresponding to '{ "list": [1, 2] }' looks > * like: > * > * <example> > * Visitor *v; > * Error *err = NULL; > * int value; > * > * v = FOO_visitor_new(...); > * visit_start_struct(v, NULL, NULL, 0, &err); > * if (err) { > * goto out; > * } > * visit_start_list(v, "list", NULL, 0, &err); > * if (err) { > * goto outobj; > * } > * value = 1; > * visit_type_int(v, NULL, &value, &err); > * if (err) { > * goto outlist; > * } > * value = 2; > * visit_type_int(v, NULL, &value, &err); > * if (err) { > * goto outlist; > * } > * outlist: > * visit_end_list(v, NULL); > * if (!err) { > * visit_check_struct(v, &err); > * } > * outobj: > * visit_end_struct(v, NULL); > * out: > * error_propagate(errp, err); > * visit_free(v); > > Why could this be useful? > > 1. With the QObject input visitor, it's an alternative way to > destructure a QObject into a bunch of C variables. The "obvious" way > would be calling the QObject accessors. By using the visitors you > get much the error checking code for free. YMMV. > > 2. With the QObject output visitor, it's an alternative way to build up > a QObject. The "obvious" way would be calling the constructors > directly. > > 3. With the string input / output visitors, it's a way to parse / format > string visitor syntax without having to construct the C type first. > > Right now, we have no virtual list walks outside tests. We do have > virtual struct walks outside tests. > >> Or mixing types >> >> visit_start_list(); >> visit_next_list(); >> visit_type_int64(); >> visit_next_list(); >> visit_type_uint64(); > > Another excellent question. > > QAPI can only express homogeneous lists, i.e. lists of some type T. > > The generated visit_type_TList call the same visit_type_T() for all list > members. Okay, my point would be: Somebody coding its own visit code should not assume this to work. > > QAPI type 'any' is the top type, but visit_type_anyList() still calls > visit_type_any() for all list members. > > Virtual walks could of course do anything. Since they don't exist > outside tests, we can outlaw doing things that cause us trouble. > > The virtual walks we currently have in tests/ seem to walk only > homogeneous lists, i.e. always call the same visit_type_T(). Okay, so bailing out if types are switched (e.g. just about to report a range of uin64_t and somebody asks for a int64_t) is valid. > >> IOW, can I assume that after every visit_next_list(), visit_type_X is >> called, and that X remains the same for one pass over the list? > > As far as I can tell, existing code is just fine with that assumption. > We'd have to write it into the contract, though. > >> Also, I assume it is supposed to work like this >> >> visit_start_list(); >> visit_next_list(); more input, returns "there's more" >> visit_type_int(); returns 1 (parses 1-3, buffers 1-3) >> visit_type_int(); returns 1 >> visit_type_int(); returns 1 >> visit_next_list(); more input, unbuffers 1 >> visit_type_int(); returns 2 >> >> So unbuffering actually happens on visit_next_list()? > > I believe this violates the contract. > > If it's a real walk, the contract wants you to call visit_next_list() > between subsequent visit_type_int(). > > If it's a virtual walk, calling visit_next_list() makes no sense. > > More questions? > Thanks for the excessive answer! I think that should be enough to come up with a RFC of a *rewrite* of the string input visitor :) -- Thanks, David / dhildenb