[tarantool-patches] Re: [PATCH 2/2] sql: refactor cursor closing routine

[Vladislav Shpilevoy <v.shpilevoy@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.