Re: [PATCH v3 5/7] box: introduce tuple_parse_iterator class

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Tue, Apr 02, 2019 at 06:49:36PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h
> index 22a0fb232..bef1d0903 100644
> --- a/src/box/tuple_format.h
> +++ b/src/box/tuple_format.h
> @@ -412,6 +412,71 @@ tuple_field_map_create(struct tuple_format *format, const char *tuple,
>  int
>  tuple_format_init();
>  
> +/**
> + * A tuple msgpack iterator that parse tuple in deep an returns

'in deep' is an idiom, which isn't appropriate in your case.
Please rephrase.

> + * only fields that are described in the tuple_format.

This isn't quite correct. Suppose I have field [1][3] indexed. Then this
iterator will return [1][1] and [1][2] in addition to [1][3] even if
[1][1] and [1][2] are not described in the format.

> + */
> +struct tuple_parse_iterator {
> +	/**
> +	 * Tuple format is used to perform field lookups in
> +	 * format::fields JSON tree.
> +	 */
> +	struct tuple_format *format;
> +	/**
> +	 * The pointer to the parent node in the format::fields
> +	 * JSON tree. Is required for relative lookup for the
> +	 * next field.
> +	 */
> +	struct json_token *parent;
> +	/**
> +	 * Traversal stack of msgpack frames is used to determine
> +	 * when the parsing of the current composite mp structure
> +	 * (array or map) is completed to update to the parent
> +	 * pointer accordingly.
> +	 */
> +	struct mp_stack stack;
> +	/** The current read position in msgpack. */
> +	const char *pos;
> +};
> +
> +/**
> + * Initialize tuple parse iterator with tuple format, data pointer
> + * and the count of top-level msgpack fields to be processed.
> + *
> + * This function assumes that the msgpack header containing the
> + * number of top-level msgpack fields (field_count) has already
> + * been parsed and the data pointer has already been shifted
> + * correspondingly. This allows directly limit the number of
> + * fields that must be parsed.

I would try to hide the first mp_decode_array inside the iterator, too,
if possible. Otherwise we have tuple decoding split between two
independent functions.

> +
> + * Function uses the region for the traversal stack allocation.
> + *
> + * Returns 0 on success. In case of memory allocation error sets
> + * diag message and returns -1.
> + */
> +int
> +tuple_parse_iterator_create(struct tuple_parse_iterator *it,
> +			    struct tuple_format *format, const char *data,
> +			    uint32_t field_count, struct region *region);
> +
> +/**
> + * Parse tuple in deep and update iterator state.
> + *
> + * Returns the number of fields at the current tuple nesting
> + * level that have been processed (2 for map item, 1 for array
> + * key:value pair, 0 on stop) and initializes:

Using the function return value to inform the caller of whether a field
is a map or an array in such an indirect way looks dubious. May be, use
a separate argument to return the type of the currently decoded
container instead (MP_MAP or MP_ARRAY)?

> + * field - the tuple_field pointer to format::fields field
> + *         that matches to the currently processed msgpack field
> + *         (when exists),
> + * data  - the pointer to the currently processed msgpack field,
> + * data_end - the pointer to the end of currently processed
> + *            msgpack field.

In case of indexed array/map field data_end points not to the end of the
current field, but to the end of array/map header. It works fine,
because data_end isn't used by the caller. Still, it looks suspicious.

> + */
> +int
> +tuple_parse_iterator_advice(struct tuple_parse_iterator *it,
> +			    struct tuple_field **field, const char **data,
> +			    const char **data_end);
> +

Not sure about the name: strictly speaking, this function doesn't parse
a tuple - it decodes it.

Also, we usually call functions subject_verb, not subject_noun.