[tarantool-patches] Re: [PATCH 1/2] sql: make aggregate functions types more strict

["n.pettik" <korablev@tarantool.org>]

Please, don’t use the same commit message within patch-set.
In this particular case, I’d say:

sql: fix code style of functions implementing min/max aggregates

> On 5 Apr 2019, at 17:57, Ivan Koptelov <ivan.koptelov@tarantool.org> wrote:
> 
> Let's firstly fix code-style.
> ---
> src/box/sql/func.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index b86a95d9a..b1bfc886e 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -1585,23 +1585,22 @@ countFinalize(sql_context * context)
>  * Routines to implement min() and max() aggregate functions.
>  */

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 379f28252..ad8e373ae 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1597,7 +1597,7 @@ countFinalize(sql_context * context)
        sql_result_int64(context, p ? p->n : 0);
 }
 
-/*
+/**
  * Routines to implement min() and max() aggregate functions.
  */
 static void

> static void
> -minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
> +minmaxStep(sql_context *context, int not_used, sql_value **argv)

FIx name of function -> minmax_agg_step()

> {
> -	Mem *pArg = (Mem *) argv[0];
> -	Mem *pBest;
> -	UNUSED_PARAMETER(NotUsed);
> +	UNUSED_PARAMETER(not_used);
> +	sql_value *arg = argv[0];
> +	sql_value *best = sql_aggregate_context(context, sizeof(*best));

What is best? Use more specific name or add comment.
I got it only after 30 minutes...

> 
> -	pBest = (Mem *) sql_aggregate_context(context, sizeof(*pBest));
> -	if (!pBest)
> +	if (best == NULL)
> 		return;
> 
> 	if (sql_value_type(argv[0]) == SQL_NULL) {
> -		if (pBest->flags)
> +		if (best->flags != 0)
> 			sqlSkipAccumulatorLoad(context);
> -	} else if (pBest->flags) {
> +	} else if (best->flags != 0) {
> 		int max;
> 		int cmp;
> -		struct coll *pColl = sqlGetFuncCollSeq(context);
> +		struct coll *coll = sqlGetFuncCollSeq(context);
> 		/* This step function is used for both the min() and max() aggregates,
> 		 * the only difference between the two being that the sense of the
> 		 * comparison is inverted. For the max() aggregate, the

Fix comment formatting:

@@ -1658,12 +1658,15 @@ minmaxStep(sql_context *context, int not_used, sql_value **argv)
                int max;
                int cmp;
                struct coll *coll = sqlGetFuncCollSeq(context);
-               /* This step function is used for both the min() and max() aggregates,
-                * the only difference between the two being that the sense of the
-                * comparison is inverted. For the max() aggregate, the
-                * sql_user_data() function returns (void *)-1. For min() it
-                * returns (void *)db, where db is the sql* database pointer.
-                * Therefore the next statement sets variable 'max' to 1 for the max()
+               /*
+                * This step function is used for both the min()
+                * and max() aggregates, the only difference
+                * between the two being that the sense of the
+                * comparison is inverted. For the max() aggregate,
+                * the sql_user_data() function returns (void *)-1.
+                * For min() it returns (void *)db, where db is the
+                * sql* database pointer. Therefore the next
+                * statement sets variable 'max' to 1 for the max()
                 * aggregate, or 0 for min().
                 */


> @@ -1611,15 +1610,15 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
> 		 * aggregate, or 0 for min().
> 		 */
> 		max = sql_user_data(context) != 0;
> -		cmp = sqlMemCompare(pBest, pArg, pColl);
> -		if ((max && cmp < 0) || (!max && cmp > 0)) {
> -			sqlVdbeMemCopy(pBest, pArg);
> +		cmp = sqlMemCompare(best, arg, coll);
> +		if ((max != 0 && cmp < 0) || (max == 0 && cmp > 0)) {
> +			sqlVdbeMemCopy(best, arg);
> 		} else {
> 			sqlSkipAccumulatorLoad(context);
> 		}
> 	} else {
> -		pBest->db = sql_context_db_handle(context);
> -		sqlVdbeMemCopy(pBest, pArg);
> +		best->db = sql_context_db_handle(context);
> +		sqlVdbeMemCopy(best, arg);
> 	}
> }
> 
> @@ -1628,8 +1627,8 @@ minMaxFinalize(sql_context * context)

Finish refactoring of this function.

> {
> 	sql_value *pRes;
> 	pRes = (sql_value *) sql_aggregate_context(context, 0);
> -	if (pRes) {
> -		if (pRes->flags) {
> +	if (pRes != NULL) {
> +		if (pRes->flags != 0) {
> 			sql_result_value(context, pRes);
> 		}
> 		sqlVdbeMemRelease(pRes);
> --