From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alyapunov@tarantool.org>
Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256
 bits)) (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id EAF9A445320
 for <tarantool-patches@dev.tarantool.org>;
 Wed, 15 Jul 2020 13:32:33 +0300 (MSK)
References: <1594221263-6228-1-git-send-email-alyapunov@tarantool.org>
 <1594221263-6228-17-git-send-email-alyapunov@tarantool.org>
 <11582023-5848-99ed-a5e8-b0d0eb6a9c3f@tarantool.org>
From: Aleksandr Lyapunov <alyapunov@tarantool.org>
Message-ID: <4c333f19-2c83-aef2-7fe9-37cc9f7e05c9@tarantool.org>
Date: Wed, 15 Jul 2020 13:32:32 +0300
MIME-Version: 1.0
In-Reply-To: <11582023-5848-99ed-a5e8-b0d0eb6a9c3f@tarantool.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Subject: Re: [Tarantool-patches] [PATCH 16/16] tx: use new tx manager in
	memtx
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>, tarantool-patches@dev.tarantool.org

Hi, thanks again for the review, see my comments below

On 15.07.2020 02:45, Vladislav Shpilevoy wrote:
> +static int
>> +memtx_engine_prepare(struct engine *engine, struct txn *txn)
>> +{
>> +	(void)engine;
>> +	struct txn_stmt *stmt;
>> +	stailq_foreach_entry(stmt, &txn->stmts, next) {
>> +		if (stmt->add_story != NULL || stmt->del_story != NULL)
> 1. Seems like it is worth moving this check into a static inline
> function in the txn.h header. You use it a lot.
It's a virtual method, it can't be inlined..
>
>> +		if (stmt->add_story != NULL || stmt->del_story != NULL)
>> +		{
> 2. Opening brace should be on the same line as 'if'. The same please
> find and fix in all the other places.
Damn change of code style. I'll try to find more.
>
>> -	struct memtx_space *memtx_space = (struct memtx_space *)space;
>> +	struct memtx_space* memtx_space = (struct memtx_space*) space;
> 3. Unnecessary diff.
Fixed IDE settings.
>   	if (stmt->engine_savepoint == NULL)
>   		return;
>   
> +	if (memtx_space->replace == memtx_space_replace_all_keys &&
> 4. Why is it done only in case of memtx_space_replace_all_keys?
Useless check, removed
>   
> -	for (uint32_t i = 0; i < index_count; i++) {
> -		struct tuple *unused;
> -		struct index *index = space->index[i];
> +	for (uint32_t i = 0; i < index_count; i++)
> +	{
> +		struct tuple* unused;
> +		struct index* index = space->index[i];
> 5. Unnecessary changes. Also not correct. We use 'type *' instead of 'type*'.
> We put { on the same line as cycle, not on a separate line. The same below
> and in all the other places.
Actually didn't found places below, but fixed.
> + if (stmt != NULL) {
>> +		return txm_history_link_stmt(txn_current_stmt(txn),
> 6. Why do you get txn_current_stmt(stmt) again? You already have it
> in stmt variable.
fixed