From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 3 Apr 2019 17:04:40 +0300 From: Vladimir Davydov Subject: Re: [PATCH v3 5/7] box: introduce tuple_parse_iterator class Message-ID: <20190403140440.zq7tfbogfjjrjto2@esperanza> References: <5d9690693bcae1856064b5eda976b56ee1137b37.1554218695.git.kshcherbatov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5d9690693bcae1856064b5eda976b56ee1137b37.1554218695.git.kshcherbatov@tarantool.org> To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: 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.