From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: "n.pettik" <korablev@tarantool.org>, tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints Date: Mon, 1 Jul 2019 23:53:15 +0200 [thread overview] Message-ID: <46ec2553-ae16-9a9f-8b85-882c2adc6031@tarantool.org> (raw) In-Reply-To: <53B73D9B-740B-469F-99DC-FF2FA14E16BA@tarantool.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;
next prev parent reply other threads:[~2019-07-01 21:52 UTC|newest] Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-07 15:37 [tarantool-patches] [PATCH 0/6] Introduce UNSIGNED type in SQL Nikita Pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 1/6] sql: refactor sql_atoi64() Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:20 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-05 16:32 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 2/6] sql: separate VDBE memory holding positive and negative ints Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:21 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-05 16:33 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:21 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy [this message] 2019-07-05 16:36 ` n.pettik 2019-07-10 22:49 ` Vladislav Shpilevoy 2019-07-17 12:24 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 4/6] sql: make built-in functions operate on unsigned values Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:21 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-05 16:36 ` n.pettik 2019-07-10 22:49 ` Vladislav Shpilevoy 2019-07-17 0:53 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 5/6] sql: introduce extended range for INTEGER type Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:21 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-24 15:59 ` Konstantin Osipov 2019-07-24 16:54 ` n.pettik 2019-07-24 17:09 ` Konstantin Osipov 2019-06-07 15:37 ` [tarantool-patches] [PATCH 6/6] sql: allow to specify UNSIGNED column type Nikita Pettik 2019-07-01 21:53 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-05 16:36 ` n.pettik 2019-07-10 22:49 ` Vladislav Shpilevoy 2019-07-11 21:25 ` Vladislav Shpilevoy 2019-07-17 0:53 ` n.pettik 2019-07-18 20:18 ` Vladislav Shpilevoy 2019-07-18 20:56 ` n.pettik 2019-07-18 21:08 ` Vladislav Shpilevoy 2019-07-18 21:13 ` Vladislav Shpilevoy 2019-07-22 10:20 ` n.pettik 2019-07-22 19:17 ` Vladislav Shpilevoy 2019-07-22 10:20 ` n.pettik 2019-07-17 0:54 ` n.pettik 2019-07-18 20:18 ` Vladislav Shpilevoy 2019-08-06 19:36 ` n.pettik 2019-07-24 13:01 ` [tarantool-patches] Re: [PATCH 0/6] Introduce UNSIGNED type in SQL Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=46ec2553-ae16-9a9f-8b85-882c2adc6031@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=korablev@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox