[Tarantool-patches] [PATCH 16/16] tx: use new tx manager in memtx
Aleksandr Lyapunov
alyapunov at tarantool.org
Wed Jul 15 13:32:32 MSK 2020
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
More information about the Tarantool-patches
mailing list