Re: [Tarantool-patches] [PATCH 16/16] tx: use new tx manager in memtx

[Aleksandr Lyapunov <alyapunov@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