[tarantool-patches] Re: [PATCH v1 3/4] sql: got rid of redundant MASKBIT32 definition

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> Thanks for the patch! See 4 comments below.
> 
> 1. We do not use the past in commit titles. Please, use 'get',
> not 'got'. In the next commit too.
Ok, done.

> 2. In the previous commit you said, that 32bit mask is already
> replaced with 64bit one where possible. But why was not it
> possible to replace this one?
The answer is in previous commit description:
"
The 32b bitmasks are still used to specify constant OP_Function
arguments because this change would require changing the P1 type
of the VDBE p1 argument, which is not desirable. Moreover, the
64 function's arguments is an explicit overkill.
"

>>   			for (i = 0; i < nFarg; i++) {
>> -				if (i < 32
>> -				    && sqlite3ExprIsConstant(pFarg->a[i].
>> -							     pExpr)) {
>> -					testcase(i == 31);
>> -					constMask |= MASKBIT32(i);
>> +				if (i < 32 &&
> 
> 3. Why 32? The column mask is smart, it has no size restrictions.
All function arguments that are not able to fit in mask to be marked as constant,
assumed to by dynamic. There is nice commit before sqlVdbeDeleteAuxData
routine definition. i < 32 must be kept to avoid calculation sqlite3ExprIsConstant
where unnecessary.
In fact such 32bit column mask is not smart, but it is not matter. Smart mask
also works fine in this scenario.

> 
>> +				    sqlite3ExprIsConstant(pFarg->a[i].pExpr)) {
>> +					column_mask_set_fieldno((uint64_t *)
>> +								&const_mask, i);
> 
> 4. column_mask_set_fieldo does *mask = ... . It means, that it rewrites 8 bytes
> from the specified address. But you passed a value of only 4 bytes, so the next
> 4 bytes of foreign memory are polluted.
You are right. It is not problem anymore.

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

MASK BIT 32 macro is now redundant, so we got rid of it.
Also refactored related code to use core bitmasks helpers

Part of #3571
---
 src/box/sql/expr.c    | 24 +++++++++++++-----------
 src/box/sql/sqlInt.h  |  1 -
 src/box/sql/vdbeaux.c |  8 +++-----
 3 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 35145cef6..a72f986c6 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3914,7 +3914,11 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 			int nFarg;	/* Number of function arguments */
 			FuncDef *pDef;	/* The function definition object */
 			const char *zId;	/* The function name */
-			u32 constMask = 0;	/* Mask of function arguments that are constant */
+			/*
+			 * Mask of function arguments that are
+			 * constant.
+			 */
+			uint32_t const_mask = 0;
 			int i;	/* Loop counter */
 			sql *db = pParse->db;	/* The database connection */
 			struct coll *coll = NULL;
@@ -3988,11 +3992,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 			}
 
 			for (i = 0; i < nFarg; i++) {
-				if (i < 32
-				    && sqlExprIsConstant(pFarg->a[i].
-							     pExpr)) {
-					testcase(i == 31);
-					constMask |= MASKBIT32(i);
+				if (i < 32 &&
+				    sqlExprIsConstant(pFarg->a[i].pExpr)) {
+					column_mask32_set_fieldno(&const_mask,
+								  i);
 				}
 				if ((pDef->funcFlags & SQL_FUNC_NEEDCOLL) !=
 				    0 && coll == NULL) {
@@ -4004,7 +4007,7 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 				}
 			}
 			if (pFarg) {
-				if (constMask) {
+				if (const_mask != 0) {
 					r1 = pParse->nMem + 1;
 					pParse->nMem += nFarg;
 				} else {
@@ -4052,12 +4055,11 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 				sqlVdbeAddOp4(v, OP_CollSeq, 0, 0, 0,
 						  (char *)coll, P4_COLLSEQ);
 			}
-			sqlVdbeAddOp4(v, OP_Function0, constMask, r1,
-					  target, (char *)pDef, P4_FUNCDEF);
+			sqlVdbeAddOp4(v, OP_Function0, const_mask, r1, target,
+				      (char *)pDef, P4_FUNCDEF);
 			sqlVdbeChangeP5(v, (u8) nFarg);
-			if (nFarg && constMask == 0) {
+			if (nFarg && const_mask == 0)
 				sqlReleaseTempRange(pParse, r1, nFarg);
-			}
 			return target;
 		}
 	case TK_EXISTS:
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 5be695e51..a9b7dfa4e 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2255,7 +2255,6 @@ typedef u64 Bitmask;
  * A bit in a Bitmask
  */
 #define MASKBIT(n)   (((Bitmask)1)<<(n))
-#define MASKBIT32(n) (((unsigned int)1)<<(n))
 #define ALLBITS      ((Bitmask)-1)
 
 /*
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index b831b52ad..fedbc0a15 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2698,11 +2698,9 @@ 	(sql * db, AuxData ** pp, int iOp, int mask)
 {
 	while (*pp) {
 		AuxData *pAux = *pp;
-		if ((iOp < 0)
-		    || (pAux->iOp == iOp
-			&& (pAux->iArg > 31 || !(mask & MASKBIT32(pAux->iArg))))
-		    ) {
-			testcase(pAux->iArg == 31);
+		if (iOp < 0 || (pAux->iOp == iOp &&
+		    (pAux->iArg >= 32 ||
+		     !column_mask32_fieldno_is_set(mask, pAux->iArg)))) {
 			if (pAux->xDelete) {
 				pAux->xDelete(pAux->pAux);
 			}
-- 
2.20.1