[PATCH v5 3/9] box: manage format fields with JSON tree class
Vladimir Davydov
vdavydov.dev at gmail.com
Thu Nov 29 22:07:57 MSK 2018
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
More information about the Tarantool-patches
mailing list