[tarantool-patches] Re: [PATCH 2/6] sql: separate VDBE memory holding positive and negative ints
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Jun 12 00:11:46 MSK 2019
Thanks for the patch! It is a masterpiece!
Excellent! Almost the best patch ever! Buuut ...
see 25 tiny minor comments below, review fixes at
the end of the email, and on the branch in a separate
commit.
On 07/06/2019 18:37, Nikita Pettik wrote:
> As it was stated in the previous commit message, we are going to support
> operations on unsigned values. Since unsigned and signed integers have
> different memory representations, to provide correct results of
> arithmetic operations we should be able to tell whether value is signed
> or not.
> This patch introduces new type of value placed in VDBE memory cell -
> MEM_UInt. This flag means that value is integer and greater than zero,
> hence can be fitted in range [0, 2^64 - 1]. Such approach would make
> further replacing MEM_* flags with MP_ format types quite easy: during
> decoding and encoding msgpack we assume that negative integers have
> MP_INT type and positive - MP_UINT. We also add and refactor several
> auxiliary helpers to operate on integers. Note that current changes
> don't add ability to operate on unsigned integers - it is still
> unavailable.
>
> Needed for #3810
> Needed for #4015
> ---
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index bb7405e68..f4c1cbcca 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -111,6 +111,7 @@ typeofFunc(sql_context * context, int NotUsed, sql_value ** argv)
> UNUSED_PARAMETER(NotUsed);
> switch (sql_value_type(argv[0])) {
> case MP_INT:
> + case MP_UINT:
1. Why on MP_UINT do you return 'integer' instead of
'unsigned'?
> z = "integer";
> break;
> case MP_STR:
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 6ecdb26fc..d141397a0 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -900,8 +904,8 @@ case OP_InitCoroutine: { /* jump */
> assert(pOp->p3>=0 && pOp->p3<p->nOp);
> pOut = &aMem[pOp->p1];
> assert(!VdbeMemDynamic(pOut));
> - pOut->u.i = pOp->p3 - 1;
> - pOut->flags = MEM_Int;
> + pOut->u.u = pOp->p3 - 1;
2. Why are you sure, that p3 >= 1? According to
the assertion above it is >= 0, but we don't know
anything about p3 >= 1.
> + pOut->flags = MEM_UInt;
> if (pOp->p2) goto jump_to_p2;
> break;
> }
> @@ -1099,7 +1108,7 @@ case OP_Bool: { /* out2 */
> case OP_Int64: { /* out2 */
> pOut = out2Prerelease(p, pOp);
> assert(pOp->p4.pI64!=0);
> - pOut->u.i = *pOp->p4.pI64;
> + mem_set_int(pOut, *pOp->p4.pI64, *pOp->p4.pI64 < 0);
3. Sorry, but such places are hell. You pass separately a value
and a flag if it is negative. Please, introduce separate functions
/* Set a value, check sign inside. */
mem_set_i64(int64_t value);
/* Set a non-negative value. */
mem_set_u64(uint64_t value);
/* Set a big integer with sign passed separately. */
mem_set_int(uint64_t value, bool is_negative);
In most places you will use mem_set_i64 and mem_set_u64. It
will look shorter and somewhere work faster.
> break;
> }
>
> @@ -1609,7 +1617,8 @@ case OP_Remainder: { /* same as TK_REM, in1, in2, out3 */
> pOut = &aMem[pOp->p3];
> flags = pIn1->flags | pIn2->flags;
> if ((flags & MEM_Null)!=0) goto arithmetic_result_is_null;
> - if ((type1 & type2 & MEM_Int)!=0) {
> + if ((type1 & (MEM_Int | MEM_UInt)) !=0 &&
> + (type2 & (MEM_Int | MEM_UInt)) !=0) {
> iA = pIn1->u.i;
> iB = pIn2->u.i;
4. How can you access pIn1/2->u.i if the types can be MEM_UInt?
> bIntint = 1;
> @@ -2485,14 +2490,14 @@ case OP_BitNot: { /* same as TK_BITNOT, in1, out2 */
> sqlVdbeMemSetNull(pOut);
> if ((pIn1->flags & MEM_Null)==0) {
> int64_t i;
> - if (sqlVdbeIntValue(pIn1, &i) != 0) {
> + bool is_neg;
> + if (sqlVdbeIntValue(pIn1, &i, &is_neg) != 0) {
> diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> sql_value_text(pIn1), "integer");
> rc = SQL_TARANTOOL_ERROR;
> goto abort_due_to_error;
> }
> - pOut->flags = MEM_Int;
> - pOut->u.i = ~i;
> + mem_set_int(pOut, ~i, ~i < 0);
5. Why do you compare 'i' with 0? What if it was a big
unsigned value? You have 'is_neg' flag for this, don't you?
> }
> break;
> }
> @@ -3507,12 +3515,16 @@ case OP_SeekGT: { /* jump, in3 */
> * the seek, so convert it.
> */
> pIn3 = &aMem[reg_ipk];
> - if ((pIn3->flags & (MEM_Int|MEM_Real|MEM_Str))==MEM_Str) {
> + if ((pIn3->flags & (MEM_Int|MEM_UInt|MEM_Real|MEM_Str))==MEM_Str) {
6. Please, remind me how is it possible, that a value is at the
same time string and number? Why not to check for flags & MEM_Str != 0
only?
> mem_apply_numeric_type(pIn3);
> }
> int64_t i;
> if ((pIn3->flags & MEM_Int) == MEM_Int) {
> i = pIn3->u.i;
> + is_neg = true;
> + } else if ((pIn3->flags & MEM_UInt) == MEM_UInt) {
> + i = pIn3->u.u;
> + is_neg = false;
> } else if ((pIn3->flags & MEM_Real) == MEM_Real) {
> if (pIn3->u.r > INT64_MAX)
> i = INT64_MAX;
> @@ -3814,10 +3828,10 @@ case OP_Sequence: { /* out2 */
> */
> case OP_NextSequenceId: {
> pOut = &aMem[pOp->p2];
> - tarantoolSqlNextSeqId((uint64_t *) &pOut->u.i);
> -
> - pOut->u.i += 1;
> - pOut->flags = MEM_Int;
> + int64_t id;
> + tarantoolSqlNextSeqId((uint64_t *) &id);
> + id++;
> + mem_set_int(pOut, id, id < 0);
7. tarantoolSqlNextSeqId returns _sequence.id field. The
field has unsigned type. How is it possible, that it is < 0?
> break;
> }
>
> @@ -5079,10 +5092,17 @@ case OP_FkIfZero: { /* jump */
> */
> case OP_IfPos: { /* jump, in1 */
> pIn1 = &aMem[pOp->p1];
> - assert(pIn1->flags&MEM_Int);
> - VdbeBranchTaken( pIn1->u.i>0, 2);
> - if (pIn1->u.i>0) {
> - pIn1->u.i -= pOp->p3;
> + assert((pIn1->flags & (MEM_Int | MEM_UInt)) != 0);
> + if ((pIn1->flags & MEM_UInt) != 0 && pIn1->u.u != 0) {
> + assert(pOp->p3 >= 0);
> + uint64_t res = pIn1->u.u - (uint64_t) pOp->p3;
> + /*
> + * To not bother setting integer flag in case
> + * result of subtraction is negative, just
> + * use saturated arithmetic.
> + */
> + res &= -(res <= pIn1->u.u);
8. I do not understand. Why do you compare res and pIn1->u.u?
You compare 'u.u' and 'u.u - p3'. Obvioiusly, the latter is
smaller. Probably, this is because you rely on 'res' being
overflowed will be bigger than 'u.u'?
In such a case, I think it would be cleaner to compare 'u.u' and
'p3'.
res &= -(u.u > p3);
Additionally, now this opcode's comment is wrong - P1 is not equal
to P1 - P3 from this moment.
> + pIn1->u.u = res;
> goto jump_to_p2;
> }
> break;
> @@ -5111,10 +5131,10 @@ case OP_OffsetLimit: { /* in1, out2, in3 */
> pIn1 = &aMem[pOp->p1];
> pIn3 = &aMem[pOp->p3];
> pOut = out2Prerelease(p, pOp);
> - assert(pIn1->flags & MEM_Int);
> - assert(pIn3->flags & MEM_Int);
> + assert(pIn1->flags & (MEM_Int | MEM_UInt));
> + assert(pIn3->flags & (MEM_Int | MEM_UInt));
> x = pIn1->u.i;
> - if (x<=0 || sqlAddInt64(&x, pIn3->u.i>0?pIn3->u.i:0)) {
> + if (x<=0 || sqlAddInt64(&x, pIn3->u.i > 0 ? pIn3->u.i : 0)) {
9. Why do you touch pIn3.u.i without checking its type? In case it
is a big unsigned, you will get 'u.i < 0'. Additionally, as I understand,
now no one should treat 'u.i' as an unsigned. It is supposed to be
negative always.
> /* If the LIMIT is less than or equal to zero, loop forever. This
> * is documented. But also, if the LIMIT+OFFSET exceeds 2^63 then
> * also loop forever. This is undocumented. In fact, one could argue
> @@ -5124,8 +5144,9 @@ case OP_OffsetLimit: { /* in1, out2, in3 */
> * looping forever is a reasonable approximation.
> */
> pOut->u.i = -1;
> + pOut->flags = MEM_Int;
> } else {
> - pOut->u.i = x;
> + mem_set_int(pOut, x, x < 0);
10. Here 'x' is always >= 0.
> }
> break;
> }
> @@ -5140,7 +5161,7 @@ case OP_OffsetLimit: { /* in1, out2, in3 */
> */
> case OP_IfNotZero: { /* jump, in1 */
> pIn1 = &aMem[pOp->p1];
> - assert(pIn1->flags&MEM_Int);
> + assert(pIn1->flags&(MEM_Int|MEM_UInt));
> VdbeBranchTaken(pIn1->u.i<0, 2);
> if (pIn1->u.i) {
> if (pIn1->u.i>0) pIn1->u.i--;
11. You can't treat 'u.i' as a positive value without
checking the flags.
> @@ -5157,7 +5178,7 @@ case OP_IfNotZero: { /* jump, in1 */
> */
> case OP_DecrJumpZero: { /* jump, in1 */
> pIn1 = &aMem[pOp->p1];
> - assert(pIn1->flags&MEM_Int);
> + assert((pIn1->flags&(MEM_Int|MEM_UInt)) != 0);
> if (pIn1->u.i>SMALLEST_INT64) pIn1->u.i--;
12. The same.
> VdbeBranchTaken(pIn1->u.i==0, 2);
> if (pIn1->u.i==0) goto jump_to_p2;
> @@ -5396,12 +5417,12 @@ case OP_Init: { /* jump */
> case OP_IncMaxid: {
> assert(pOp->p1 > 0);
> pOut = &aMem[pOp->p1];
> -
> - rc = tarantoolsqlIncrementMaxid((uint64_t*) &pOut->u.i);
> + int64_t id;
> + rc = tarantoolsqlIncrementMaxid((uint64_t*) &id);
13. Id is always non-negative.
> if (rc!=SQL_OK) {
> goto abort_due_to_error;
> }
> - pOut->flags = MEM_Int;
> + mem_set_int(pOut, id, id < 0);
> break;
> }
>
> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index a3100e513..abed46486 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> @@ -259,9 +261,14 @@ struct Mem {
> * auxiliary flags.
> */
> enum {
> - MEM_PURE_TYPE_MASK = 0x3f
> + MEM_PURE_TYPE_MASK = 0x7f
> };
>
> +static_assert((int) MEM_PURE_TYPE_MASK == (MEM_Null | MEM_Str | MEM_Int |
14. Why do you need the cast to 'int'?
> + MEM_Real | MEM_Blob | MEM_Bool |
> + MEM_UInt),
> + "value of type mask must consist of corresponding to memory type bits");
15. Out of 80.
> +
> /**
> * Simple type to str convertor. It is used to simplify
> * error reporting.
> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index e480ae720..393782c23 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -233,7 +234,8 @@ sql_int64
> sql_value_int64(sql_value * pVal)
> {
> int64_t i = 0;
> - sqlVdbeIntValue((Mem *) pVal, &i);
> + bool is_neg;
> + sqlVdbeIntValue((Mem *) pVal, &i, &is_neg);
> return i;
16. The result integer is invalid in case it was a
big unsigned value.
> }
>
> @@ -385,7 +388,7 @@ sql_result_error(sql_context * pCtx, const char *z, int n)
> void
> sql_result_int(sql_context * pCtx, int iVal)
> {
> - sqlVdbeMemSetInt64(pCtx->pOut, (i64) iVal);
> + mem_set_int(pCtx->pOut, (i64) iVal, iVal < 0);
17. int to int64 cast works implicitly. You don't need an
explicit cast.
> }
>
> void
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index 25d4cd759..5a71e1801 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -1639,6 +1641,8 @@ sqlVdbeList(Vdbe * p)
> if (p->explain == 1) {
> pMem->flags = MEM_Int;
> pMem->u.i = i; /* Program counter */
> + mem_set_int(pMem, i, i < 0);
> +
> pMem++;
18. Stray empty line + you do mem_set_int() twice - once you
call the function, and second you set flag and value manually.
Besides, program counter is always > 0.
>
> pMem->flags = MEM_Static | MEM_Str | MEM_Term;
> @@ -3395,10 +3403,23 @@ sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl)
> return -1;
> }
> }
> + if ((f1 & MEM_UInt) != 0) {
> + if ((f2 & MEM_Real) != 0) {
> + return sqlIntFloatCompare(pMem1->u.i,
> + pMem2->u.r);
> + } else if ((f2 & MEM_Int) != 0) {
> + return +1;
> + } else {
> + return -1;
> + }
> + }
> if ((f1 & MEM_Real) != 0) {
> if ((f2 & MEM_Int) != 0) {
> return -sqlIntFloatCompare(pMem2->u.i,
> pMem1->u.r);
> + } else if ((f2 & MEM_UInt) != 0) {
> + return -sqlIntFloatCompare(pMem2->u.u,
19. You treat 'u.u' here as int64_t. Why?
> + pMem1->u.r);
> } else {
> return -1;
> }
> @@ -3563,13 +3584,14 @@ sqlVdbeCompareMsgpack(const char **key1,
> mem1.u.r = (double)v;
> goto do_float;
> }
> - mem1.u.i = v;
> + mem1.u.u = v;
> goto do_int;
> }
> case MP_INT:{
> mem1.u.i = mp_decode_int(&aKey1);
> do_int:
> - if (pKey2->flags & MEM_Int) {
> + if ((pKey2->flags & MEM_Int) ||
> + (pKey2->flags & MEM_UInt)) {
> if (mem1.u.i < pKey2->u.i) {
20. How can you touch pKey2->u.i, if you don't know
if it is MEM_Int or MEM_UInt? The only thing you know is
that it is an integer. You don't know which.
> rc = -1;
> } else if (mem1.u.i > pKey2->u.i) {
> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> index 3a361d066..25119ff16 100644
> --- a/src/box/sql/vdbemem.c
> +++ b/src/box/sql/vdbemem.c
> @@ -418,7 +422,7 @@ sqlVdbeMemRelease(Mem * p)
> * return the closest available 64-bit signed integer.
> */
> static int
> -doubleToInt64(double r, int64_t *i)
> +doubleToInt64(double r, int64_t *i, bool *is_neg)
21. I understand 'is_neg' in the most of other places,
but why here? In a caller function you can always check
'r < 0' if necessary.
> {
> /*
> * Many compilers we encounter do not define constants for the
> @@ -454,20 +458,24 @@ doubleToInt64(double r, int64_t *i)
> * If pMem represents a string value, its encoding might be changed.
> */
> int
> -sqlVdbeIntValue(Mem * pMem, int64_t *i)
> +sqlVdbeIntValue(Mem * pMem, int64_t *i, bool *is_neg)
> {
> int flags;
> assert(EIGHT_BYTE_ALIGNMENT(pMem));
> flags = pMem->flags;
> if (flags & MEM_Int) {
> *i = pMem->u.i;
> + *is_neg = true;
> + return 0;
> + } else if (flags & MEM_UInt) {
> + *i = pMem->u.u;
> + *is_neg = false;
> return 0;
> } else if (flags & MEM_Real) {
> - return doubleToInt64(pMem->u.r, i);
> + return doubleToInt64(pMem->u.r, i, is_neg);
> } else if (flags & (MEM_Str)) {
> assert(pMem->z || pMem->n == 0);
> - bool is_neg;
> - if (sql_atoi64(pMem->z, (int64_t *)i, &is_neg, pMem->n) == 0)
> + if (sql_atoi64(pMem->z, (int64_t *)i, is_neg, pMem->n) == 0)
22. I do not understand these casts. 'i' is already int64_t *. Why do
you cast it?
> return 0;
> }
> return -1;
> @@ -489,7 +497,11 @@ sqlVdbeRealValue(Mem * pMem, double *v)
> } else if (pMem->flags & MEM_Int) {
> *v = (double)pMem->u.i;
> return 0;
> - } else if (pMem->flags & MEM_Str) {
> + } else if (pMem->flags & MEM_UInt) {
> + *v = (double)pMem->u.u;
> + return 0;
> + }
> + else if (pMem->flags & MEM_Str) {
23. Stray line wrap.
> if (sqlAtoF(pMem->z, v, pMem->n))
> return 0;
> }
> @@ -777,42 +792,29 @@ sqlVdbeMemSetZeroBlob(Mem * pMem, int n)
> pMem->z = 0;
> }
>
> -/*
> - * The pMem is known to contain content that needs to be destroyed prior
> - * to a value change. So invoke the destructor, then set the value to
> - * a 64-bit integer.
> - */
> -static SQL_NOINLINE void
> -vdbeReleaseAndSetInt64(Mem * pMem, i64 val)
> +void
> +mem_set_bool(struct Mem *mem, bool value)
24. Please, move this function back below mem_set_int to
avoid diff padding out.
> {
> - sqlVdbeMemSetNull(pMem);
> - pMem->u.i = val;
> - pMem->flags = MEM_Int;
> + sqlVdbeMemSetNull(mem);
> + mem->u.b = value;
> + mem->flags = MEM_Bool;
> }
>
> -/*
> - * Delete any previous value and set the value stored in *pMem to val,
> - * manifest type INTEGER.
> - */
> void
> -sqlVdbeMemSetInt64(Mem * pMem, i64 val)
> +mem_set_int(struct Mem *mem, int64_t value, bool is_neg)
> {
> - if (VdbeMemDynamic(pMem)) {
> - vdbeReleaseAndSetInt64(pMem, val);
> + if (VdbeMemDynamic(mem))
> + sqlVdbeMemSetNull(mem);
> + if (is_neg) {
> + assert(value < 0);
> + mem->u.i = value;
> + MemSetTypeFlag(mem, MEM_Int);
> } else {
> - pMem->u.i = val;
> - pMem->flags = MEM_Int;
> + mem->u.u = value;
> + MemSetTypeFlag(mem, MEM_UInt);
> }
> }
>
> -void
> -mem_set_bool(struct Mem *mem, bool value)
> -{
> - sqlVdbeMemSetNull(mem);
> - mem->u.b = value;
> - mem->flags = MEM_Bool;
> -}
25. I was confused by several places where you use Mem.u.i not
as a negative integer, but as a buffer of both integers. To avoid
further confusion I think we should add another alias to Mem.u -
'ib' - integer buffer. It should be uint64_t, but due to its name
it will be clear that in places of its usage we just want to take
raw bytes of the stored integer, separately from its sign.
Besides, in a couple of places 'ib' usage will be faster.
1) sqlVdbeMemCast - here the only thing which matters if 'ib' is 0.
Now you check it twice - for 'u.u' and 'u.i'.
2) OP_SeekGT, here:
if ((pIn3->flags & MEM_Int) == MEM_Int) {
i = pIn3->u.i;
is_neg = true;
} else if ((pIn3->flags & MEM_UInt) == MEM_UInt) {
i = pIn3->u.u;
is_neg = false;
In this place you could write 'i = pIn3->u.ib; is_neg = flags == MEM_Int;'.
Please, consider my review fixes here and on the branch in a separate commit:
=============================================================
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7e7c12ee4..dffdcf55d 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -347,10 +347,9 @@ mem_apply_type(struct Mem *record, enum field_type type)
* an integer or real representation (BLOB and
* NULL do not get converted).
*/
- if ((record->flags & MEM_Str) == 0) {
- if ((record->flags & (MEM_Real | MEM_Int | MEM_UInt)))
- sqlVdbeMemStringify(record, 1);
- }
+ if ((record->flags & MEM_Str) == 0 &&
+ (record->flags & (MEM_Real | MEM_Int | MEM_UInt)) != 0)
+ sqlVdbeMemStringify(record, 1);
record->flags &= ~(MEM_Real | MEM_Int | MEM_UInt);
return 0;
case FIELD_TYPE_SCALAR:
@@ -380,7 +379,7 @@ sql_value_apply_type(
*/
static u16 SQL_NOINLINE computeNumericType(Mem *pMem)
{
- assert((pMem->flags & (MEM_Int | MEM_UInt | MEM_Real))==0);
+ assert((pMem->flags & (MEM_Int | MEM_UInt | MEM_Real)) == 0);
assert((pMem->flags & (MEM_Str|MEM_Blob))!=0);
if (sqlAtoF(pMem->z, &pMem->u.r, pMem->n)==0)
return 0;
@@ -399,9 +398,8 @@ static u16 SQL_NOINLINE computeNumericType(Mem *pMem)
*/
static u16 numericType(Mem *pMem)
{
- if (pMem->flags & (MEM_Int| MEM_UInt | MEM_Real)) {
+ if ((pMem->flags & (MEM_Int | MEM_UInt | MEM_Real)) != 0)
return pMem->flags & (MEM_Int | MEM_UInt | MEM_Real);
- }
if (pMem->flags & (MEM_Str|MEM_Blob)) {
return computeNumericType(pMem);
}
@@ -861,9 +859,7 @@ case OP_Gosub: { /* jump */
pIn1 = &aMem[pOp->p1];
assert(VdbeMemDynamic(pIn1)==0);
memAboutToChange(p, pIn1);
- pIn1->flags = MEM_UInt;
- assert((pOp-aOp) >= 0);
- pIn1->u.u = (int)(pOp-aOp);
+ mem_set_int(pIn1, pOp - aOp, false);
REGISTER_TRACE(pOp->p1, pIn1);
/* Most jump operations do a goto to this spot in order to update
@@ -904,8 +900,7 @@ case OP_InitCoroutine: { /* jump */
assert(pOp->p3>=0 && pOp->p3<p->nOp);
pOut = &aMem[pOp->p1];
assert(!VdbeMemDynamic(pOut));
- pOut->u.u = pOp->p3 - 1;
- pOut->flags = MEM_UInt;
+ mem_set_int(pOut, pOp->p3 - 1, false);
if (pOp->p2) goto jump_to_p2;
break;
}
@@ -945,12 +940,10 @@ case OP_EndCoroutine: { /* in1 */
* See also: InitCoroutine
*/
case OP_Yield: { /* in1, jump */
- int pcDest;
pIn1 = &aMem[pOp->p1];
assert(VdbeMemDynamic(pIn1)==0);
- pIn1->flags = MEM_UInt;
- pcDest = (int)pIn1->u.u;
- pIn1->u.u = (int)(pOp - aOp);
+ int pcDest = (int)pIn1->u.u;
+ mem_set_int(pIn1, pOp - aOp, false);
REGISTER_TRACE(pOp->p1, pIn1);
pOp = &aOp[pcDest];
break;
@@ -1078,12 +1071,10 @@ case OP_Halt: {
*/
case OP_Integer: { /* out2 */
pOut = out2Prerelease(p, pOp);
- if (pOp->p1 < 0) {
+ if (pOp->p1 < 0)
pOut->u.i = pOp->p1;
- } else {
- pOut->u.u = pOp->p1;
- MemSetTypeFlag(pOut, MEM_UInt);
- }
+ else
+ mem_set_int(pOut, pOp->p1, false);
break;
}
@@ -1392,9 +1383,9 @@ case OP_SCopy: { /* out2 */
*/
case OP_IntCopy: { /* out2 */
pIn1 = &aMem[pOp->p1];
- assert((pIn1->flags & (MEM_Int | MEM_UInt))!=0);
+ assert((pIn1->flags & (MEM_Int | MEM_UInt)) != 0);
pOut = &aMem[pOp->p2];
- mem_set_int(pOut, pIn1->u.i, pIn1->flags & MEM_Int);
+ mem_set_int(pOut, pIn1->u.i, (pIn1->flags & MEM_Int) != 0);
break;
}
@@ -1617,8 +1608,8 @@ case OP_Remainder: { /* same as TK_REM, in1, in2, out3 */
pOut = &aMem[pOp->p3];
flags = pIn1->flags | pIn2->flags;
if ((flags & MEM_Null)!=0) goto arithmetic_result_is_null;
- if ((type1 & (MEM_Int | MEM_UInt)) !=0 &&
- (type2 & (MEM_Int | MEM_UInt)) !=0) {
+ if ((type1 & (MEM_Int | MEM_UInt)) != 0 &&
+ (type2 & (MEM_Int | MEM_UInt)) != 0) {
iA = pIn1->u.i;
iB = pIn2->u.i;
bIntint = 1;
@@ -1954,7 +1945,7 @@ case OP_MustBeInt: { /* jump, in1 */
pIn1 = &aMem[pOp->p1];
if ((pIn1->flags & (MEM_Int | MEM_UInt)) == 0) {
mem_apply_type(pIn1, FIELD_TYPE_INTEGER);
- if ((pIn1->flags & (MEM_Int | MEM_UInt))== 0) {
+ if ((pIn1->flags & (MEM_Int | MEM_UInt)) == 0) {
if (pOp->p2==0) {
rc = SQL_MISMATCH;
goto abort_due_to_error;
@@ -2171,12 +2162,12 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */
enum field_type type = pOp->p5 & FIELD_TYPE_MASK;
if (sql_type_is_numeric(type)) {
if ((flags1 | flags3)&MEM_Str) {
- if ((flags1 & (MEM_Int|MEM_UInt|MEM_Real|MEM_Str))==MEM_Str) {
+ if ((flags1 & MEM_Str) == MEM_Str) {
mem_apply_numeric_type(pIn1);
testcase( flags3!=pIn3->flags); /* Possible if pIn1==pIn3 */
flags3 = pIn3->flags;
}
- if ((flags3 & (MEM_Int|MEM_UInt|MEM_Real|MEM_Str))==MEM_Str) {
+ if ((flags3 & MEM_Str) == MEM_Str) {
if (mem_apply_numeric_type(pIn3) != 0) {
diag_set(ClientError,
ER_SQL_TYPE_MISMATCH,
@@ -2191,14 +2182,16 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */
/* Handle the common case of integer comparison here, as an
* optimization, to avoid a call to sqlMemCompare()
*/
- if ((pIn1->flags & pIn3->flags & (MEM_Int | MEM_UInt))!=0) {
+ if ((pIn1->flags & pIn3->flags &
+ (MEM_Int | MEM_UInt)) != 0) {
if (pIn3->u.i > pIn1->u.i) { res = +1; goto compare_op; }
if (pIn3->u.i < pIn1->u.i) { res = -1; goto compare_op; }
res = 0;
goto compare_op;
}
} else if (type == FIELD_TYPE_STRING) {
- if ((flags1 & MEM_Str)==0 && (flags1 & (MEM_Int|MEM_UInt|MEM_Real))!=0) {
+ if ((flags1 & MEM_Str) == 0 &&
+ (flags1 & (MEM_Int | MEM_UInt | MEM_Real)) != 0) {
testcase( pIn1->flags & MEM_Int);
testcase( pIn1->flags & MEM_Real);
sqlVdbeMemStringify(pIn1, 1);
@@ -2206,7 +2199,8 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */
flags1 = (pIn1->flags & ~MEM_TypeMask) | (flags1 & MEM_TypeMask);
assert(pIn1!=pIn3);
}
- if ((flags3 & MEM_Str)==0 && (flags3 & (MEM_Int|MEM_UInt|MEM_Real))!=0) {
+ if ((flags3 & MEM_Str) == 0 &&
+ (flags3 & (MEM_Int | MEM_UInt | MEM_Real)) != 0) {
testcase( pIn3->flags & MEM_Int);
testcase( pIn3->flags & MEM_Real);
sqlVdbeMemStringify(pIn3, 1);
@@ -2755,7 +2749,7 @@ case OP_Column: {
pDest->flags = MEM_Blob|MEM_Ephem|MEM_Subtype;
pDest->subtype = SQL_SUBTYPE_MSGPACK;
}
- if ((pDest->flags & (MEM_Int|MEM_UInt)) != 0 &&
+ if ((pDest->flags & (MEM_Int | MEM_UInt)) != 0 &&
pC->eCurType == CURTYPE_TARANTOOL) {
enum field_type f = FIELD_TYPE_ANY;
/*
@@ -2959,9 +2953,7 @@ case OP_Count: { /* out2 */
}
if (rc) goto abort_due_to_error;
pOut = out2Prerelease(p, pOp);
- assert(nEntry >= 0);
- pOut->u.u = nEntry;
- pOut->flags = MEM_UInt;
+ mem_set_int(pOut, nEntry, false);
break;
}
@@ -3515,9 +3507,9 @@ case OP_SeekGT: { /* jump, in3 */
* the seek, so convert it.
*/
pIn3 = &aMem[reg_ipk];
- if ((pIn3->flags & (MEM_Int|MEM_UInt|MEM_Real|MEM_Str))==MEM_Str) {
+ if ((pIn3->flags & (MEM_Int | MEM_UInt | MEM_Real |
+ MEM_Str)) == MEM_Str)
mem_apply_numeric_type(pIn3);
- }
int64_t i;
if ((pIn3->flags & MEM_Int) == MEM_Int) {
i = pIn3->u.i;
@@ -3813,9 +3805,7 @@ case OP_Sequence: { /* out2 */
assert(p->apCsr[pOp->p1]!=0);
pOut = out2Prerelease(p, pOp);
int64_t seq_val = p->apCsr[pOp->p1]->seqCount++;
- assert(seq_val >= 0);
- pOut->u.u = seq_val;
- pOut->flags = MEM_UInt;
+ mem_set_int(pOut, seq_val, false);
break;
}
@@ -3864,8 +3854,7 @@ case OP_NextIdEphemeral: {
goto abort_due_to_error;
}
pOut = &aMem[pOp->p2];
- pOut->u.u = rowid;
- pOut->flags = MEM_UInt;
+ mem_set_int(pOut, rowid, false);
break;
}
@@ -3900,8 +3889,7 @@ case OP_FCopy: { /* out2 */
assert((pIn1->flags & (MEM_Int | MEM_UInt)) != 0);
pOut = &aMem[pOp->p2];
- mem_set_int(pOut, pIn1->u.i, pIn1->flags == MEM_UInt ?
- false : true);
+ mem_set_int(pOut, pIn1->u.i, pIn1->flags == MEM_Int);
}
break;
}
@@ -5131,8 +5119,8 @@ case OP_OffsetLimit: { /* in1, out2, in3 */
pIn1 = &aMem[pOp->p1];
pIn3 = &aMem[pOp->p3];
pOut = out2Prerelease(p, pOp);
- assert(pIn1->flags & (MEM_Int | MEM_UInt));
- assert(pIn3->flags & (MEM_Int | MEM_UInt));
+ assert((pIn1->flags & (MEM_Int | MEM_UInt)) != 0);
+ assert((pIn3->flags & (MEM_Int | MEM_UInt)) != 0);
x = pIn1->u.i;
if (x<=0 || sqlAddInt64(&x, pIn3->u.i > 0 ? pIn3->u.i : 0)) {
/* If the LIMIT is less than or equal to zero, loop forever. This
@@ -5143,8 +5131,7 @@ case OP_OffsetLimit: { /* in1, out2, in3 */
* it would take nearly 300 years to actually reach the limit. So
* looping forever is a reasonable approximation.
*/
- pOut->u.i = -1;
- pOut->flags = MEM_Int;
+ mem_set_int(pOut, -1, true);
} else {
mem_set_int(pOut, x, x < 0);
}
@@ -5161,7 +5148,7 @@ case OP_OffsetLimit: { /* in1, out2, in3 */
*/
case OP_IfNotZero: { /* jump, in1 */
pIn1 = &aMem[pOp->p1];
- assert(pIn1->flags&(MEM_Int|MEM_UInt));
+ assert((pIn1->flags & (MEM_Int | MEM_UInt)) != 0);
VdbeBranchTaken(pIn1->u.i<0, 2);
if (pIn1->u.i) {
if (pIn1->u.i>0) pIn1->u.i--;
@@ -5178,7 +5165,7 @@ case OP_IfNotZero: { /* jump, in1 */
*/
case OP_DecrJumpZero: { /* jump, in1 */
pIn1 = &aMem[pOp->p1];
- assert((pIn1->flags&(MEM_Int|MEM_UInt)) != 0);
+ assert((pIn1->flags & (MEM_Int | MEM_UInt)) != 0);
if (pIn1->u.i>SMALLEST_INT64) pIn1->u.i--;
VdbeBranchTaken(pIn1->u.i==0, 2);
if (pIn1->u.i==0) goto jump_to_p2;
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index abed46486..d29f3323b 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -231,7 +231,7 @@ struct Mem {
#define MEM_Real 0x0008 /* Value is a real number */
#define MEM_Blob 0x0010 /* Value is a BLOB */
#define MEM_Bool 0x0020 /* Value is a bool */
-#define MEM_UInt 0x0040 /* Value is a unsigned integer */
+#define MEM_UInt 0x0040 /* Value is an unsigned integer */
#define MEM_Frame 0x0080 /* Value is a VdbeFrame object */
#define MEM_Undefined 0x0100 /* Value is undefined */
#define MEM_Cleared 0x0200 /* NULL set by OP_Null, not from data */
@@ -264,10 +264,10 @@ enum {
MEM_PURE_TYPE_MASK = 0x7f
};
-static_assert((int) MEM_PURE_TYPE_MASK == (MEM_Null | MEM_Str | MEM_Int |
- MEM_Real | MEM_Blob | MEM_Bool |
- MEM_UInt),
- "value of type mask must consist of corresponding to memory type bits");
+static_assert(MEM_PURE_TYPE_MASK == (MEM_Null | MEM_Str | MEM_Int | MEM_Real |
+ MEM_Blob | MEM_Bool | MEM_UInt),
+ "value of type mask must consist of corresponding to memory "\
+ "type bits");
/**
* Simple type to str convertor. It is used to simplify
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 393782c23..3c0a9cc59 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -388,7 +388,7 @@ sql_result_error(sql_context * pCtx, const char *z, int n)
void
sql_result_int(sql_context * pCtx, int iVal)
{
- mem_set_int(pCtx->pOut, (i64) iVal, iVal < 0);
+ mem_set_int(pCtx->pOut, iVal, iVal < 0);
}
void
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 5a71e1801..de5fa5047 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1639,9 +1639,8 @@ sqlVdbeList(Vdbe * p)
pOp = &apSub[j]->aOp[i];
}
if (p->explain == 1) {
- pMem->flags = MEM_Int;
- pMem->u.i = i; /* Program counter */
- mem_set_int(pMem, i, i < 0);
+ assert(i >= 0);
+ mem_set_int(pMem, i, false);
pMem++;
@@ -3373,7 +3372,7 @@ sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl)
/* At least one of the two values is a number
*/
- if (combined_flags & (MEM_Int | MEM_UInt | MEM_Real)) {
+ if ((combined_flags & (MEM_Int | MEM_UInt | MEM_Real)) != 0) {
if ((f1 & f2 & MEM_Int) != 0) {
if (pMem1->u.i < pMem2->u.i)
return -1;
@@ -3590,8 +3589,7 @@ sqlVdbeCompareMsgpack(const char **key1,
case MP_INT:{
mem1.u.i = mp_decode_int(&aKey1);
do_int:
- if ((pKey2->flags & MEM_Int) ||
- (pKey2->flags & MEM_UInt)) {
+ if ((pKey2->flags & (MEM_Int | MEM_UInt)) != 0) {
if (mem1.u.i < pKey2->u.i) {
rc = -1;
} else if (mem1.u.i > pKey2->u.i) {
@@ -3612,7 +3610,7 @@ sqlVdbeCompareMsgpack(const char **key1,
case MP_DOUBLE:{
mem1.u.r = mp_decode_double(&aKey1);
do_float:
- if (pKey2->flags & MEM_Int || pKey2->flags & MEM_UInt) {
+ if ((pKey2->flags & (MEM_Int | MEM_UInt)) != 0) {
rc = -sqlIntFloatCompare(pKey2->u.i,
mem1.u.r);
} else if (pKey2->flags & MEM_Real) {
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 48cb556e9..9b70e61a4 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -291,7 +291,7 @@ sqlVdbeMemStringify(Mem * pMem, u8 bForce)
return SQL_OK;
assert(!(fg & MEM_Zero));
- assert(fg & (MEM_Int | MEM_UInt | MEM_Real | MEM_Bool));
+ assert((fg & (MEM_Int | MEM_UInt | MEM_Real | MEM_Bool)) != 0);
assert(EIGHT_BYTE_ALIGNMENT(pMem));
if (sqlVdbeMemClearAndResize(pMem, nByte)) {
@@ -497,11 +497,10 @@ sqlVdbeRealValue(Mem * pMem, double *v)
} else if (pMem->flags & MEM_Int) {
*v = (double)pMem->u.i;
return 0;
- } else if (pMem->flags & MEM_UInt) {
+ } else if ((pMem->flags & MEM_UInt) != 0) {
*v = (double)pMem->u.u;
return 0;
- }
- else if (pMem->flags & MEM_Str) {
+ } else if (pMem->flags & MEM_Str) {
if (sqlAtoF(pMem->z, v, pMem->n))
return 0;
}
@@ -596,9 +595,9 @@ sqlVdbeMemNumerify(Mem * pMem)
if ((pMem->flags & (MEM_Int | MEM_UInt | MEM_Real | MEM_Null)) == 0) {
assert((pMem->flags & (MEM_Blob | MEM_Str)) != 0);
bool is_neg;
- if (sql_atoi64(pMem->z, &pMem->u.i, &is_neg, pMem->n) == 0) {
- int flag = is_neg ? MEM_Int : MEM_UInt;
- MemSetTypeFlag(pMem, flag);
+ int64_t i;
+ if (sql_atoi64(pMem->z, &i, &is_neg, pMem->n) == 0) {
+ mem_set_int(pMem, i, is_neg);
} else {
double v;
if (sqlVdbeRealValue(pMem, &v))
More information about the Tarantool-patches
mailing list