[tarantool-patches] Re: [PATCH v2 2/4] sql: refactor getNewIid() function

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch! See 4 comments below, review fixes
at the end of the email, and on the branch.

On 28/03/2019 15:07, Nikita Pettik wrote:
> This commit includes no functional changes. Lets simply rewrite
> getNewIid() function according to Tarantool codestyle.
> 
> Part of #3914
> ---
>  src/box/sql/build.c | 72 ++++++++++++++++++++++++++---------------------------
>  1 file changed, 35 insertions(+), 37 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 8fb001433..f55f6d800 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1918,45 +1918,43 @@ sql_drop_foreign_key(struct Parse *parse_context)
>  	sqlVdbeChangeP5(sqlGetVdbe(parse_context), OPFLAG_NCHANGE);
>  }
>  
> -/*
> - * Generate code to determine next free Iid in the space identified by
> - * the iSpaceId. Return register number holding the result.
> +/**
> + * Generate code to determine next free index id in
> + * the space identified by the @space_id.

1. According to doxygen docs http://www.doxygen.nl/manual/commands.html
there is no such a command as @space_id. But obviously I know what
you've implied here. For references to parameters we use '@a' before a
parameter. Like this:

    ...
    * the space identifier by @a space_id.
    ...

> + * Return register holding the result.
> + *
> + * Overall VDBE program logic is following:
> + *
> + * 1 Seek for space id in _index, goto l1 if seeks fails.
> + * 2 Goto l2.
> + * 3 l1: Halt.
> + * 4 l2: Fetch index id from _index record.
>   */
>  static int
> -getNewIid(Parse * pParse, int iSpaceId, int iCursor)
> +generate_index_id(struct Parse *parse, uint32_t space_id, int cursor)

2. For vdbe methods generating code we use prefix 'vdbe_emit_'. And you
use it in the last patch btw.

3. In fact, that function generates an index_id for only secondary
indexes. Taking into account these two comments I renamed it to
vdbe_emit_new_sec_index_id().

>  {
> -	Vdbe *v = sqlGetVdbe(pParse);
> -	int iRes = ++pParse->nMem;
> -	int iKey = ++pParse->nMem;
> -	int iSeekInst, iGotoInst;
> -
> -	sqlVdbeAddOp2(v, OP_Integer, iSpaceId, iKey);
> -	iSeekInst = sqlVdbeAddOp4Int(v, OP_SeekLE, iCursor, 0, iKey, 1);
> -	sqlVdbeAddOp4Int(v, OP_IdxLT, iCursor, 0, iKey, 1);
> -
> -	/*
> -	 * If SeekLE succeeds, the control falls through here, skipping
> -	 * IdxLt.
> -	 *
> -	 * If it fails (no entry with the given key prefix: invalid spaceId)
> -	 * VDBE jumps to the next code block (jump target is IMM, fixed up
> -	 * later with sqlVdbeJumpHere()).
> -	 */
> -	iGotoInst = sqlVdbeAddOp0(v, OP_Goto);	/* Jump over Halt */
> -
> -	/* Invalid spaceId detected. Halt now. */
> -	sqlVdbeJumpHere(v, iSeekInst);
> -	sqlVdbeJumpHere(v, iSeekInst + 1);
> -	sqlVdbeAddOp4(v,
> -			  OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, 0,
> -			  sqlMPrintf(pParse->db, "Invalid space id: %d",
> -					 iSpaceId), P4_DYNAMIC);
> -
> -	/* Fetch iid from the row and ++it. */
> -	sqlVdbeJumpHere(v, iGotoInst);
> -	sqlVdbeAddOp3(v, OP_Column, iCursor, 1, iRes);
> -	sqlVdbeAddOp2(v, OP_AddImm, iRes, 1);
> -	return iRes;
> +	struct Vdbe *v = sqlGetVdbe(parse);
> +	int key_reg = ++parse->nMem;
> +
> +	sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg);
> +	int seek_adr = sqlVdbeAddOp4Int(v, OP_SeekLE, cursor, 0,
> +					    key_reg, 1);

4. Indentation.

> +	sqlVdbeAddOp4Int(v, OP_IdxLT, cursor, 0, key_reg, 1);
> +	/* Jump over Halt block. */
> +	int goto_succ_addr = sqlVdbeAddOp0(v, OP_Goto);
> +	/* Invalid space id handling block starts here. */
> +	sqlVdbeJumpHere(v, seek_adr);
> +	sqlVdbeJumpHere(v, seek_adr + 1);
> +	sqlVdbeAddOp4(v, OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, 0,
> +		      sqlMPrintf(parse->db, "Invalid space id: %d", space_id),
> +		      P4_DYNAMIC);
> +

Note that I deliberately used @return instead of @retval, because
@return has no arguments - it is just a description.

=================================================================

commit 9e77985d09e53332d87b323c6ef74b747c418d8e
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Thu Mar 28 15:53:24 2019 +0300

    Review fix

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index f55f6d800..b86fcb68d 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1919,27 +1919,28 @@ sql_drop_foreign_key(struct Parse *parse_context)
 }
 
 /**
- * Generate code to determine next free index id in
- * the space identified by the @space_id.
- * Return register holding the result.
- *
- * Overall VDBE program logic is following:
+ * Generate code to determine next free secondary index id in the
+ * space identified by @a space_id. Overall VDBE program logic is
+ * following:
  *
  * 1 Seek for space id in _index, goto l1 if seeks fails.
  * 2 Goto l2.
  * 3 l1: Halt.
  * 4 l2: Fetch index id from _index record.
+ *
+ * @return Register holding a new index id.
  */
 static int
-generate_index_id(struct Parse *parse, uint32_t space_id, int cursor)
+vdbe_emit_new_sec_index_id(struct Parse *parse, uint32_t space_id,
+			   int _index_cursor)
 {
 	struct Vdbe *v = sqlGetVdbe(parse);
 	int key_reg = ++parse->nMem;
 
 	sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg);
-	int seek_adr = sqlVdbeAddOp4Int(v, OP_SeekLE, cursor, 0,
-					    key_reg, 1);
-	sqlVdbeAddOp4Int(v, OP_IdxLT, cursor, 0, key_reg, 1);
+	int seek_adr = sqlVdbeAddOp4Int(v, OP_SeekLE, _index_cursor, 0,
+					key_reg, 1);
+	sqlVdbeAddOp4Int(v, OP_IdxLT, _index_cursor, 0, key_reg, 1);
 	/* Jump over Halt block. */
 	int goto_succ_addr = sqlVdbeAddOp0(v, OP_Goto);
 	/* Invalid space id handling block starts here. */
@@ -1952,7 +1953,7 @@ generate_index_id(struct Parse *parse, uint32_t space_id, int cursor)
 	sqlVdbeJumpHere(v, goto_succ_addr);
 	/* Fetch iid from the row and increment it. */
 	int iid_reg = ++parse->nMem;
-	sqlVdbeAddOp3(v, OP_Column, cursor, BOX_INDEX_FIELD_ID, iid_reg);
+	sqlVdbeAddOp3(v, OP_Column, _index_cursor, BOX_INDEX_FIELD_ID, iid_reg);
 	sqlVdbeAddOp2(v, OP_AddImm, iid_reg, 1);
 	return iid_reg;
 }
@@ -2414,7 +2415,8 @@ sql_create_index(struct Parse *parse) {
 				  (void *)space_by_id(BOX_INDEX_ID),
 				  P4_SPACEPTR);
 		sqlVdbeChangeP5(vdbe, OPFLAG_SEEKEQ);
-		int index_id = generate_index_id(parse, def->id, cursor);
+		int index_id = vdbe_emit_new_sec_index_id(parse, def->id,
+							  cursor);
 		sqlVdbeAddOp1(vdbe, OP_Close, cursor);
 		vdbe_emit_create_index(parse, def, index->def, def->id, index_id);
 		sqlVdbeChangeP5(vdbe, OPFLAG_NCHANGE);