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 DD8132A3EF for ; Mon, 1 Apr 2019 16:43:29 -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 477Ws8AyujQF for ; Mon, 1 Apr 2019 16:43:29 -0400 (EDT) Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 87EB729B38 for ; Mon, 1 Apr 2019 16:43:29 -0400 (EDT) From: Stanislav Zudin Subject: [tarantool-patches] Re: [PATCH 07/13] sql: arithmetic functions support big integers References: Message-ID: <071a569b-de79-1afe-7335-44de04d6a867@tarantool.org> Date: Mon, 1 Apr 2019 23:43:27 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: tarantool-patches@freelists.org, "n.pettik" On 25.03.2019 18:13, n.pettik wrote: > >> Makes arithmetic functions accept arguments with >> values in the range [2^63, 2^64). >> --- >> src/box/sql/func.c | 2 +- >> src/box/sql/sqlInt.h | 23 +++- >> src/box/sql/util.c | 236 ++++++++++++++++++++++++++++++++---------- >> src/box/sql/vdbe.c | 36 ++++--- >> src/box/sql/vdbeInt.h | 2 +- >> 5 files changed, 223 insertions(+), 76 deletions(-) >> >> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h >> index 9b1d7df9a..7f8e3f04e 100644 >> --- a/src/box/sql/sqlInt.h >> +++ b/src/box/sql/sqlInt.h >> @@ -4383,9 +4383,26 @@ Expr *sqlExprAddCollateString(Parse *, Expr *, const char *); >> Expr *sqlExprSkipCollate(Expr *); >> int sqlCheckIdentifierName(Parse *, char *); >> void sqlVdbeSetChanges(sql *, int); >> -int sqlAddInt64(i64 *, i64); >> -int sqlSubInt64(i64 *, i64); >> -int sqlMulInt64(i64 *, i64); >> + >> +enum arithmetic_result { >> + /* The result fits the signed 64-bit integer */ >> + ATHR_SIGNED, >> + /* The result is positive and fits the >> + * unsigned 64-bit integer >> + */ >> + ATHR_UNSIGNED, >> + /* The operation causes an overflow */ >> + ATHR_OVERFLOW, >> + /* The operation causes division by zero */ >> + ATHR_DIVBYZERO >> +}; >> + >> +enum arithmetic_result sqlAddInt64(i64 *, bool, i64, bool); >> +enum arithmetic_result sqlSubInt64(i64 *, bool, i64, bool); >> +enum arithmetic_result sqlMulInt64(i64 *, bool, i64, bool); >> +enum arithmetic_result sqlDivInt64(i64 *, bool, i64, bool); >> +enum arithmetic_result sqlRemInt64(i64 *, bool, i64, bool); > > Since you’ve already fixed signature of these functions, > please make them follow Tarantool code style: > > enum arithmetic_result > sql_add_int64(int64_t *lhs, bool is_lhs_signed, …); Done. > > What is more, personally I would apply the same fix as for atoi functions: > make them return -1 in case of overflow or division by 0 and set > diag message right in these functions; use enum to represent their args. It's a bad practice to write diagnostic from the low-level functions. Using a single enum as a return value gives a compact readable code. > >> + >> int sqlAbsInt32(int); >> #ifdef SQL_ENABLE_8_3_NAMES >> void sqlFileSuffix3(const char *, char *); >> diff --git a/src/box/sql/util.c b/src/box/sql/util.c >> index be77f72f8..3786c5083 100644 >> --- a/src/box/sql/util.c >> +++ b/src/box/sql/util.c >> @@ -1249,74 +1249,202 @@ sqlSafetyCheckSickOrOk(sql * db) >> } >> >> +/* >> + * Attempt to add, substract, or multiply the 64-bit value iB against >> + * the other 64-bit integer at *pA and store the result in *pA. >> + * Return ATHR_SIGNED or ATHR_UNSIGNED on success. >> + * Or if the operation would have resulted in an >> + * overflow, leave *pA unchanged and return ATHR_OVERFLOW. >> + */ >> +enum arithmetic_result >> +sqlAddInt64(i64 * pA, bool is_signedA, i64 iB, bool is_signedB) >> { >> i64 iA = *pA; >> - testcase(iA == 0); >> - testcase(iA == 1); >> - testcase(iB == -1); >> - testcase(iB == 0); >> - if (iB >= 0) { >> - testcase(iA > 0 && LARGEST_INT64 - iA == iB); >> - testcase(iA > 0 && LARGEST_INT64 - iA == iB - 1); >> - if (iA > 0 && LARGEST_INT64 - iA < iB) >> - return 1; >> + >> + bool is_negA = iA < 0 && is_signedA; >> + bool is_negB = iB < 0 && is_signedB; >> + >> + /* Make sure we've got only one combination of >> + * positive and negative operands >> + */ > > Nit: note that correct way of comment formatting is: > > /* > * Make sure we've got only one combination of > * positive and negative operands. > */ > Fixed. >> /* >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >> index ea9d9d98f..d4bd845fb 100644 >> --- a/src/box/sql/vdbe.c >> +++ b/src/box/sql/vdbe.c >> @@ -1672,28 +1672,29 @@ case OP_Remainder: { /* same as TK_REM, in1, in2, out3 */ >> if ((type1 & type2 & MEM_Int)!=0) { >> iA = pIn1->u.i; >> iB = pIn2->u.i; >> + bool is_signedA = (type1 & MEM_Unsigned) == 0; >> + bool is_signedB = (type2 & MEM_Unsigned) == 0; >> bIntint = 1; >> + enum arithmetic_result arr; >> switch( pOp->opcode) { >> - case OP_Add: if (sqlAddInt64(&iB,iA)) goto integer_overflow; break; >> - case OP_Subtract: if (sqlSubInt64(&iB,iA)) goto integer_overflow; break; >> - case OP_Multiply: if (sqlMulInt64(&iB,iA)) goto integer_overflow; break; >> - case OP_Divide: { >> - if (iA == 0) >> - goto division_by_zero; >> - if (iA==-1 && iB==SMALLEST_INT64) goto integer_overflow; >> - iB /= iA; >> - break; >> + case OP_Add: arr = sqlAddInt64(&iB, is_signedA, iA, is_signedB); break; >> + case OP_Subtract: arr = sqlSubInt64(&iB, is_signedA, iA, is_signedB); break; >> + case OP_Multiply: arr = sqlMulInt64(&iB, is_signedA, iA, is_signedB); break; >> + case OP_Divide: arr = sqlDivInt64(&iB, is_signedA, iA, is_signedB); break; >> + default: arr = sqlRemInt64(&iB, is_signedA, iA, is_signedB); break; > > SQL ANSI specifications doesn’t provide description of unsigned behaviour. > But for example in C there is no unsigned overflow, because if result can’t > be represented by unsigned range, it is truncated to modulo (MAX_UINT + 1 == 1). > Should we follow this way? IDK, it needs discussion involving other team members. Good point. > >