[tarantool-patches] Re: [PATCH 1/2] sql: make aggregate functions types more strict
n.pettik
korablev at tarantool.org
Tue Apr 9 17:52:39 MSK 2019
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 at 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);
> --
More information about the Tarantool-patches
mailing list