From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tarantool-patches-bounce@freelists.org>
Received: from localhost (localhost [127.0.0.1])
	by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B258B290FD
	for <tarantool-patches@freelists.org>; Thu, 12 Apr 2018 09:28:49 -0400 (EDT)
Received: from turing.freelists.org ([127.0.0.1])
	by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024)
	with ESMTP id 3wh7KyDExpeE for <tarantool-patches@freelists.org>;
	Thu, 12 Apr 2018 09:28:49 -0400 (EDT)
Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(No client certificate requested)
	by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id E45EC28F38
	for <tarantool-patches@freelists.org>; Thu, 12 Apr 2018 09:28:48 -0400 (EDT)
Subject: [tarantool-patches] Re: [PATCH 2/2] sql: refactor cursor closing
 routine
References: <cover.1523468339.git.korablev@tarantool.org>
 <7410c7c9cec4772039931a8086fa1bb60b24d705.1523468339.git.korablev@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <0c626bc2-a3c0-c4b9-e2ab-cee9ac481051@tarantool.org>
Date: Thu, 12 Apr 2018 16:28:44 +0300
MIME-Version: 1.0
In-Reply-To: <7410c7c9cec4772039931a8086fa1bb60b24d705.1523468339.git.korablev@tarantool.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: tarantool-patches-bounce@freelists.org
Errors-to: tarantool-patches-bounce@freelists.org
Reply-To: tarantool-patches@freelists.org
List-help: <mailto:ecartis@freelists.org?Subject=help>
List-unsubscribe: <tarantool-patches-request@freelists.org?Subject=unsubscribe>
List-software: Ecartis version 1.0.0
List-Id: tarantool-patches <tarantool-patches.freelists.org>
List-subscribe: <tarantool-patches-request@freelists.org?Subject=subscribe>
List-owner: <mailto:>
List-post: <mailto:tarantool-patches@freelists.org>
List-archive: <http://www.freelists.org/archives/tarantool-patches>
To: tarantool-patches@freelists.org, Nikita Pettik <korablev@tarantool.org>

See below a pair of comments.

> diff --git a/src/box/sql/cursor.c b/src/box/sql/cursor.c
> index 533bfb587..c38629f3b 100644
> --- a/src/box/sql/cursor.c
> +++ b/src/box/sql/cursor.c
> @@ -31,18 +31,26 @@
>   
>   #include "sqliteInt.h"
>   #include "tarantoolInt.h"
> +#include "box/tuple.h"
>   
> -/*
> - * Clear the current cursor position.
> +/**
> + * Release tuple, free iterator, invalidate cursor's state.
> + * Note that this routine doesn't nullify space and index:
> + * it is also used during OP_NullRow opcode to refresh given
> + * cursor.
>    */
>   void
> -sqlite3ClearCursor(BtCursor * pCur)
> +sql_cursor_cleanup(struct BtCursor *cursor)
>   {
> -	free(pCur->key);
> -	pCur->key = 0;
> -	pCur->iter = NULL;
> -	pCur->last_tuple = NULL;
> -	pCur->eState = CURSOR_INVALID;
> +	if (cursor->iter)
> +		box_iterator_free(cursor->iter);

1. If you use internal structures like struct iterator, then lets
manage them using internal API - iterator_delete(). I know, that
box_iterator_t is the same as struct iterator, but lets be
consistent.
> diff --git a/src/box/sql/cursor.h b/src/box/sql/cursor.h
> index 5b8e7c29d..e5d2aae3a 100644
> --- a/src/box/sql/cursor.h
> +++ b/src/box/sql/cursor.h
> @@ -72,14 +72,16 @@ struct BtCursor {
>   void sqlite3CursorZero(BtCursor *);
>   void sqlite3CursorHintFlags(BtCursor *, unsigned);
>   
> -int sqlite3CloseCursor(BtCursor *);
> +void
> +sql_cursor_close(struct BtCursor *cursor);

2. Lets store comments near to function declarations rather
than implementations.