[tarantool-patches] Re: [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes!

On 05/07/2019 18:36, n.pettik wrote:
> 
> 
>> On 2 Jul 2019, at 00:53, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>
>> Thanks for the fixes!
>>
>>>>> @@ -5134,7 +5175,11 @@ case OP_OffsetLimit: {    /* in1, out2, in3 */
>>>>> 	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)) {
>>>>> +	int64_t rhs = pIn3->flags & MEM_Int ? 0 : pIn3->u.u;
>>>>> +	bool unused;
>>>>> +	if ((x == 0 || pIn1->flags & MEM_Int) ||
>>>>> +	    sql_add_int(x, pIn1->flags & MEM_Int, rhs, false,
>>>>
>>>> 14. If you get to this line, then (pIn1->flags & MEM_Int) is already
>>>> 0 and can be inlined.
>>>
>>> Wait, why? If x == 0 then pIn1->flags == MEM_UInt -
>>> we consider 0 as an unsigned value.
>>
>> Because you can only get to sql_add_int(), if x != 0 and
>> pIn1->flags & MEM_Int == 0. It is the C standard. In an
>> expression (a || b) 'b' is executed iff 'a' is false.
> 
> My bad, never mind.
> 
>> Looks like that place is not tested at all. The tests pass
>> regardless of how I call sql_add_int: with pIn1->flags & MEM_Int -> false
>> or true.
> 
> In fact, the reason is the same as for OP_DecrJumpZero and
> OP_IfNotZero: P1 is a limit counter and P3 is an offset counter.
> Hence, both are >= 0, it is checked before these opcodes are
> executed. Added fix to the previous patch:
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index f864ef950..81005d14a 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4929,18 +4929,16 @@ case OP_IfPos: {        /* jump, in1 */
>   * Otherwise, r[P2] is set to the sum of r[P1] and r[P3].
>   */
>  case OP_OffsetLimit: {    /* in1, out2, in3 */
> -       i64 x;
>         pIn1 = &aMem[pOp->p1];
>         pIn3 = &aMem[pOp->p3];
>         pOut = out2Prerelease(p, pOp);
> -       assert((pIn1->flags & (MEM_Int | MEM_UInt)) != 0);
> -       assert((pIn3->flags & (MEM_Int | MEM_UInt)) != 0);
> -       x = pIn1->u.i;
> -       int64_t rhs = pIn3->flags & MEM_Int ? 0 : pIn3->u.u;
> +       assert((pIn1->flags & MEM_UInt) != 0);
> +       assert((pIn3->flags & MEM_UInt) != 0);
> +       uint64_t x = pIn1->u.u;
> +       uint64_t rhs = pIn3->u.u;
>         bool unused;
> -       if ((x == 0 || pIn1->flags & MEM_Int) ||
> -           sql_add_int(x, pIn1->flags & MEM_Int, rhs, false,
> -                       (int64_t *) &x, &unused) != 0) {
> +       if (x == 0 || sql_add_int(x, false, rhs, false, (int64_t *) &x,
> +                                 &unused) != 0) {
> 
>> Please, inline the value (false), and add a test, which would fail, if
>> I put here true.
>>

I did this:

@@ -4943,7 +4943,7 @@ case OP_OffsetLimit: {    /* in1, out2, in3 */
 	uint64_t x = pIn1->u.u;
 	uint64_t rhs = pIn3->u.u;
 	bool unused;
-	if (x == 0 || sql_add_int(x, false, rhs, false, (int64_t *) &x,
+	if (x == 0 || sql_add_int(x, true, rhs, false, (int64_t *) &x,
 				  &unused) != 0) {
 		/* 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

And the tests passed. Looks like a test was not added.