From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id DE54B22BB6 for ; Mon, 1 Jul 2019 17:52:16 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id bVm_2yzMycQd for ; Mon, 1 Jul 2019 17:52:16 -0400 (EDT) Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 9E4D422B80 for ; Mon, 1 Jul 2019 17:52:16 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints References: <7d8e776bc82cb9d25460c2104e687540526aa7cd.1559919361.git.korablev@tarantool.org> <9233795e-a77c-565a-9bd5-3712499e7fce@tarantool.org> <53B73D9B-740B-469F-99DC-FF2FA14E16BA@tarantool.org> From: Vladislav Shpilevoy Message-ID: <46ec2553-ae16-9a9f-8b85-882c2adc6031@tarantool.org> Date: Mon, 1 Jul 2019 23:53:15 +0200 MIME-Version: 1.0 In-Reply-To: <53B73D9B-740B-469F-99DC-FF2FA14E16BA@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: "n.pettik" , tarantool-patches@freelists.org 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;