On 10/20/2016 10:24 AM, Peter Krempa wrote:
Simplifies cases where JSON array members need to be transferred to a different structure. --- src/libvirt_private.syms | 1 + src/util/virjson.c | 36 ++++++++++++++++++++++++++++++++++++ src/util/virjson.h | 6 ++++++ 3 files changed, 43 insertions(+)
+/** + * virJSONValueArrayForeachSteal: + * @array: array to iterate + * @cb: callback called on every member of the array + * @opaque: custom data for the callback + * + * Iterates members of the array and calls the callback on every single member. + * By returning success, the array claims ownership of given array element and
s/array claims/callback claims/ "returning success" - is that 0?
+ * is responsible for freeing it. + * + * Returns 0 if all members were successfully stolen by the callback; -1 + * otherwise. The rest of the members stays in posession of the array.
s/stays/stay/ s/posession/the possession/ Does the iteration stop at the first callback that returns non-zero? Do we want something a bit more complex, where a negative return aborts right away, returning 0 means the callback claimed possession but keep iterating, and where a positive return means the callback did not claim possession but wants to keep iterating?
+ */ +int +virJSONValueArrayForeachSteal(virJSONValuePtr array, + virJSONArrayIteratorFunc cb, + void *opaque) +{ + ssize_t i; + + if (array->type != VIR_JSON_TYPE_ARRAY) + return -1; + + for (i = array->data.array.nvalues - 1; i >= 0; i--) {
Does order of iteration matter? JSON arrays (can be) order-dependent, unlike dicts that are order-agnostic. All other clients of data.array.nvalues iterate forward, not backward, so we ought to do likewise.
+ if (cb(i, array->data.array.values[i], opaque) < 0) + break;
Okay, you are handling < 0 as failure and end iteration, and treating all non-negative as successfully claiming the element. But is that enough power for all uses of this interface? See my thoughts above about distinguishing between 0 and positive returns. Then again, if you do that, you'd have to memmove remaining elements to close gaps.
+ + array->data.array.values[i] = NULL; + } + + array->data.array.nvalues = i + 1;
I see why you iterate backwards; you don't have to memmove elements that aren't stolen. But I'm still not convinced it is the best thing to be doing.
+ + return i < 0 ? 0 : -1; +}
The interface sounds like it will be useful, but I want to make sure it doesn't lock us into design issues that will haunt us later on. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org