From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <kshcherbatov@tarantool.org>
Subject: Re: [tarantool-patches] Re: [PATCH v5 1/4] box: introduce
 tuple_format_iterator class
References: <cover.1557142159.git.kshcherbatov@tarantool.org>
 <0e9e576a96eadd8bf20de1ffe3635ac543d9de9c.1557142159.git.kshcherbatov@tarantool.org>
 <20190506135555.wgtewxgznfta3uss@esperanza>
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Message-ID: <e74b5dbf-cc9f-6d9b-97fc-10a0042ff01b@tarantool.org>
Date: Mon, 6 May 2019 17:32:37 +0300
MIME-Version: 1.0
In-Reply-To: <20190506135555.wgtewxgznfta3uss@esperanza>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
To: tarantool-patches@freelists.org, Vladimir Davydov <vdavydov.dev@gmail.com>
List-ID: <tarantool-patches.dev.tarantool.org>

>> +	uint32_t total_sz = frames_sz + 2 * it->required_fields_sz;
> 
> 2 * ?
> 
> Shouldn't this be a part of the next patch?

Yep. Moved to the last patch.

>> +	mp_stack_create(&it->stack, format->fields_depth, frames);
>> +	*defined_field_count = MIN(*defined_field_count, validate ?
> 
> Shouldn't we use KEY_PARTS_ONLY here rather than VALIDATE?

Yes, this works.

>> +/**
>> + * Scan required_fields bitmap and raise error when it is
>> + * non-empty.
>> + * @sa format:required_fields and field:multikey_required_fields
>> + * definition.
> 
> There's no such thing as multikey_required_fields yet.

Moved to the last patch.

>> +	case MP_MAP:
>> +		if (mp_typeof(*it->pos) != MP_STR) {
>> +			entry->data = it->pos;
>> +			entry->field = NULL;
>> +			mp_next(&it->pos);
>> +			mp_next(&it->pos);
> 
> What about entry->data_end?

Both of them are redundant, entry->data must be only != NULL.
But I've updated data_end correspondingly:

case MP_MAP:
		if (mp_typeof(*it->pos) != MP_STR) {
			entry->field = NULL;
			mp_next(&it->pos);
			entry->data = it->pos;
			mp_next(&it->pos);
			entry->data_end = it->pos;
			return 0;
		}

==============================================

Updated patch on branch.