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

[Vladislav Shpilevoy]

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.

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.

Please, inline the value (false), and add a test, which would fail, if
I put here true.

> 	if (is_lhs_neg) {
> 		uint64_t u_lhs = (uint64_t) (-lhs);
> 		uint64_t u_rhs = is_rhs_neg ? (uint64_t) (-rhs) :
> 				 (uint64_t) rhs;
> 		uint64_t u_res = u_lhs % u_rhs;
> 		if (u_res > (uint64_t) INT64_MAX + 1)
> 			return -1;

Please, add a test for this error. I've removed that check,
and the tests passed.

Consider new fixes below, and on the branch
in a separate commit.

=====================================================

diff --git a/src/box/sql/util.c b/src/box/sql/util.c
index d58c0c6e6..1bdaa24e5 100644
--- a/src/box/sql/util.c
+++ b/src/box/sql/util.c
@@ -952,7 +952,6 @@ sql_add_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg,
 	/* Addition of two negative integers. */
 	if (is_lhs_neg && is_rhs_neg) {
 		assert(lhs < 0 && rhs < 0);
-		/* This is the same as (lhs + rhs) < INT64_MIN */
 		if (lhs < INT64_MIN - rhs)
 				return -1;
 		*is_res_neg = true;
@@ -963,7 +962,6 @@ sql_add_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg,
 	if (!is_lhs_neg && !is_rhs_neg) {
 		uint64_t u_lhs = (uint64_t) lhs;
 		uint64_t u_rhs = (uint64_t) rhs;
-		/* This is the same as (lhs + rhs) > UINT64_MAX */
 		if (UINT64_MAX - u_lhs < u_rhs)
 			return -1;
 		*is_res_neg = false;
@@ -1001,7 +999,7 @@ sql_sub_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg,
 	assert(is_lhs_neg && !is_rhs_neg);
 	/*
 	 * (lhs - rhs) < 0, lhs < 0, rhs > 0: in this case their
-	 * difference must be less than INT64_MIN.
+	 * difference must not be less than INT64_MIN.
 	 */
 	if ((uint64_t) -lhs + (uint64_t) rhs > (uint64_t) INT64_MAX + 1)
 		return -1;
@@ -1104,10 +1102,9 @@ int
 sql_rem_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg,
 	    int64_t *res, bool *is_res_neg)
 {
+	uint64_t u_rhs = is_rhs_neg ? (uint64_t) (-rhs) : (uint64_t) rhs;
 	if (is_lhs_neg) {
 		uint64_t u_lhs = (uint64_t) (-lhs);
-		uint64_t u_rhs = is_rhs_neg ? (uint64_t) (-rhs) :
-				 (uint64_t) rhs;
 		uint64_t u_res = u_lhs % u_rhs;
 		if (u_res > (uint64_t) INT64_MAX + 1)
 			return -1;
@@ -1120,7 +1117,6 @@ sql_rem_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg,
 	 * rhs - it doesn't affect the result.
 	 * */
 	uint64_t u_lhs = (uint64_t) lhs;
-	uint64_t u_rhs = is_rhs_neg ? (uint64_t) (-rhs) : (uint64_t) rhs;
 	*res = u_lhs % u_rhs;
 	*is_res_neg = false;
 	return 0;