[tarantool-patches] Re: [PATCH v1 3/4] sql: got rid of redundant MASKBIT32 definition
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Feb 15 20:05:08 MSK 2019
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) {
More information about the Tarantool-patches
mailing list