From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 29 Nov 2018 22:07:57 +0300 From: Vladimir Davydov Subject: Re: [PATCH v5 3/9] box: manage format fields with JSON tree class Message-ID: <20181129190757.oyu7kom6drgacd3j@esperanza> References: <0ff5601b664e42d70d71be658b4ba45fe1237564.1543229303.git.kshcherbatov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0ff5601b664e42d70d71be658b4ba45fe1237564.1543229303.git.kshcherbatov@tarantool.org> To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org, kostja@tarantool.org List-ID: On Mon, Nov 26, 2018 at 01:49:37PM +0300, Kirill Shcherbatov wrote: > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c > index d184dba..92028c5 100644 > --- a/src/box/tuple_format.c > +++ b/src/box/tuple_format.c > @@ -38,10 +38,28 @@ static intptr_t recycled_format_ids = FORMAT_ID_NIL; > > static uint32_t formats_size = 0, formats_capacity = 0; > > -static const struct tuple_field tuple_field_default = { > - FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false, > - ON_CONFLICT_ACTION_NONE, NULL, COLL_NONE, > -}; > +static struct tuple_field * > +tuple_field_create(struct json_token *token) > +{ > + struct tuple_field *ret = calloc(1, sizeof(struct tuple_field)); > + if (ret == NULL) { > + diag_set(OutOfMemory, sizeof(struct tuple_field), "malloc", > + "ret"); > + return NULL; > + } > + ret->type = FIELD_TYPE_ANY; > + ret->offset_slot = TUPLE_OFFSET_SLOT_NIL; > + ret->coll_id = COLL_NONE; > + ret->nullable_action = ON_CONFLICT_ACTION_NONE; > + ret->token = *token; > + return ret; > +} > + > +static void > +tuple_field_destroy(struct tuple_field *field) > +{ > + free(field); > +} We usually call methods that allocate/free an object _new/delete, not _create/destroy. Please fix. > @@ -258,39 +273,68 @@ tuple_format_alloc(struct key_def * const *keys, uint16_t key_count, > } > } > uint32_t field_count = MAX(space_field_count, index_field_count); > - uint32_t total = sizeof(struct tuple_format) + > - field_count * sizeof(struct tuple_field); > > - struct tuple_format *format = (struct tuple_format *) malloc(total); > + struct tuple_format *format = malloc(sizeof(struct tuple_format)); > if (format == NULL) { > diag_set(OutOfMemory, sizeof(struct tuple_format), "malloc", > "tuple format"); > return NULL; > } > + if (json_tree_create(&format->tree) != 0) { > + free(format); > + return NULL; > + } > + struct json_token token; > + memset(&token, 0, sizeof(token)); > + token.key.type = JSON_TOKEN_NUM; > + for (token.key.num = TUPLE_INDEX_BASE; > + token.key.num < field_count + TUPLE_INDEX_BASE; token.key.num++) { > + struct tuple_field *field = tuple_field_create(&token); I've looked at your following patches - this is the only place where you pass a non-NULL token to tuple_field_create. Let's not pass token to this function at all, and initialize field->token directly here. > + if (field == NULL) > + goto error; > + if (json_tree_add(&format->tree, NULL, &field->token) != 0) { diag_set missing > + tuple_field_destroy(field); > + goto error; > + } > + } > if (dict == NULL) { > assert(space_field_count == 0); > format->dict = tuple_dictionary_new(NULL, 0); > - if (format->dict == NULL) { > - free(format); > - return NULL; > - } > + if (format->dict == NULL) > + goto error; > } else { > format->dict = dict; > tuple_dictionary_ref(dict); > } > format->refs = 0; > format->id = FORMAT_ID_NIL; > - format->field_count = field_count; > format->index_field_count = index_field_count; > format->exact_field_count = 0; > format->min_field_count = 0; > return format; > +error:; > + struct tuple_field *field; > + json_tree_foreach_entry_safe(field, &format->tree.root, > + struct tuple_field, token) { > + json_tree_del(&format->tree, &field->token); > + tuple_field_destroy(field); > + } > + json_tree_destroy(&format->tree); And you have exactly the same piece of code in tuple_format_destroy. Factor it out to a helper function? > + free(format); > + return NULL; > } > > /** Free tuple format resources, doesn't unregister. */ > static inline void > tuple_format_destroy(struct tuple_format *format) > { > + struct tuple_field *field; > + json_tree_foreach_entry_safe(field, &format->tree.root, > + struct tuple_field, token) { > + json_tree_del(&format->tree, &field->token); > + tuple_field_destroy(field); > + } > + json_tree_destroy(&format->tree); > tuple_dictionary_unref(format->dict); > } > > diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h > index 232df22..2da773b 100644 > --- a/src/box/tuple_format.h > +++ b/src/box/tuple_format.h > @@ -34,6 +34,7 @@ > #include "key_def.h" > #include "field_def.h" > #include "errinj.h" > +#include "json/json.h" > #include "tuple_dictionary.h" > > #if defined(__cplusplus) > @@ -113,6 +114,8 @@ struct tuple_field { > struct coll *coll; > /** Collation identifier. */ > uint32_t coll_id; > + /** An JSON entry to organize tree. */ /** Link in tuple_format::fields. */ would be more useful. > + struct json_token token; > }; > > /** > @@ -166,16 +169,33 @@ struct tuple_format { > * index_field_count <= min_field_count <= field_count. > */ > uint32_t min_field_count; > - /* Length of 'fields' array. */ > - uint32_t field_count; > /** > * Shared names storage used by all formats of a space. > */ > struct tuple_dictionary *dict; > - /* Formats of the fields */ > - struct tuple_field fields[0]; > + /** JSON tree of fields. */ Would be prudent to say a few words about the tree structure, e.g. /** * Fields comprising the format, organized in a tree. * First level nodes correspond to tuple fields. * Deeper levels define indexed JSON paths within * tuple fields. Nodes of the tree are linked by * tuple_field::token. */ I hate to repeat myself, but you ought to pay more attention to comments in the code. > + struct json_tree tree; format->tree - what tree? Why not leave the same name - 'fields'? > }; > > + > +static inline uint32_t > +tuple_format_field_count(const struct tuple_format *format) > +{ > + return format->tree.root.child_count; > +} > + Comments, comments... > +static inline struct tuple_field * > +tuple_format_field(struct tuple_format *format, uint32_t fieldno) > +{ > + assert(fieldno < tuple_format_field_count(format)); > + struct json_token token = { > + .key.type = JSON_TOKEN_NUM, > + .key.num = fieldno + TUPLE_INDEX_BASE > + }; > + return json_tree_lookup_entry(&format->tree, NULL, &token, > + struct tuple_field, token); > +} > + > extern struct tuple_format **tuple_formats; > > static inline uint32_t