[tarantool-patches] Re: [PATCH 5/6] sql: introduce extended range for INTEGER type

[Vladislav Shpilevoy]

Thanks for the patch! See 1 comment below, review
fixes at the end of the email, and on the branch in
a separate commit.

> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index f4aba2f54..417029adb 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -242,7 +242,7 @@ struct errcode_record {
>  	/*187 */_(ER_SQL_ANALYZE_ARGUMENT,	"ANALYZE statement argument %s is not a base table") \
>  	/*188 */_(ER_SQL_COLUMN_COUNT_MAX,	"Failed to create space '%s': space column count %d exceeds the limit (%d)") \
>  	/*189 */_(ER_HEX_LITERAL_MAX,		"Hex literal %s%s length %d exceeds the supported limit (%d)") \
> -	/*190 */_(ER_INT_LITERAL_MAX,		"Integer literal %s exceeds the supported range %lld - %lld") \
> +	/*190 */_(ER_INT_LITERAL_MAX,		"Integer literal %s exceeds the supported range %lld - %llu") \

That error is used only once and with the constants. Please, inline them, and
make that error argument-less. Also, alongside I propose you to write the range in
[start, end] instead of 'start - end'. The latter looks like a minus.

Please, consider my review fixes here and on the branch in a separate commit:

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

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 75d6579ca..93aab8501 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -918,10 +918,10 @@ emitNewSysSequenceRecord(Parse *pParse, int reg_seq_id, const char *seq_name)
 
 	/* 5. Minimum  */
 	sqlVdbeAddOp4Dup8(v, OP_Int64, 0, first_col + 5, 0,
-			      (unsigned char*)&min_usigned_long_long, P4_UINT64);
+			  (unsigned char *) &min_usigned_long_long, P4_UINT64);
 	/* 6. Maximum  */
 	sqlVdbeAddOp4Dup8(v, OP_Int64, 0, first_col + 6, 0,
-			      (unsigned char*)&max_usigned_long_long, P4_UINT64);
+			  (unsigned char *) &max_usigned_long_long, P4_UINT64);
 	/* 7. Start  */
 	sqlVdbeAddOp2(v, OP_Integer, 1, first_col + 7);