[tarantool-patches] Re: [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Jul 11 01:49:21 MSK 2019
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 at 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.
More information about the Tarantool-patches
mailing list