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

[Vladislav Shpilevoy <v.shpilevoy@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.

On 08/02/2019 13:52, Kirill Shcherbatov wrote:
> 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      | 22 ++++++++++++----------
>   src/box/sql/sqliteInt.h |  1 -
>   src/box/sql/vdbeaux.c   |  7 +++----
>   3 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index d83be5101..cf82bd366 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -3963,7 +3963,11 @@ sqlite3ExprCodeTarget(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;


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?

>   			int i;	/* Loop counter */
>   			sqlite3 *db = pParse->db;	/* The database connection */
>   			struct coll *coll = NULL;
> @@ -4038,11 +4042,10 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>   			}
>   
>   			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.

> +				    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.

>   				}
>   				if ((pDef->funcFlags & SQLITE_FUNC_NEEDCOLL) !=
>   				    0 && coll == NULL) {