Re: [PATCH v5 3/9] box: manage format fields with JSON tree class

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

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