From: Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v5 46/52] sql: introduce mem_get_uint() Date: Wed, 14 Apr 2021 03:39:48 +0300 [thread overview] Message-ID: <20210414003948.GA140789@tarantool.org> (raw) In-Reply-To: <29a16259-d26a-4391-4a8d-0439fec146a0@tarantool.org> Thank you for the review! My answer, diff and new patch below. On Wed, Apr 14, 2021 at 01:04:05AM +0200, Vladislav Shpilevoy wrote: > Thanks for the fixes! > > On 09.04.2021 23:08, Mergen Imeev via Tarantool-patches wrote: > > Thank you for the review! My answer and new patch below. > > > > > > On 30.03.2021 02:08, Vladislav Shpilevoy wrote: > >> Thanks for the patch! > >> > >> On 23.03.2021 10:36, Mergen Imeev via Tarantool-patches wrote: > >>> This patch introduces mem_get_unsigned() function which is used to > >>> receive unsigned value from MEM. > >>> > >>> Part of #5818 > >>> --- > >>> src/box/sql/func.c | 16 +++++++++++----- > >>> src/box/sql/mem.c | 37 +++++++++++++++++++++++++++---------- > >>> src/box/sql/mem.h | 6 +++--- > >>> src/box/sql/sqlInt.h | 3 --- > >>> src/box/sql/vdbeapi.c | 6 ------ > >>> 5 files changed, 41 insertions(+), 27 deletions(-) > >>> > >>> diff --git a/src/box/sql/func.c b/src/box/sql/func.c > >>> index 0fa0f6ac7..a851d98f2 100644 > >>> --- a/src/box/sql/func.c > >>> +++ b/src/box/sql/func.c > >>> @@ -118,9 +118,12 @@ port_vdbemem_dump_lua(struct port *base, struct lua_State *L, bool is_flat) > >>> luaL_pushint64(L, n); > >>> break; > >>> } > >>> - case MP_UINT: > >>> - luaL_pushuint64(L, sql_value_uint64(param)); > >>> + case MP_UINT: { > >>> + uint64_t u; > >>> + mem_get_unsigned(param, &u); > >>> + luaL_pushuint64(L, u); > >> Maybe we could make 2 functions? One to get the value and ignore > >> the errors, and the other to get as an out parameter + return an > >> error? > >> > >> For instance, mem_to_uint() - returns uint64_t and internally asserts > >> that the value is correct. And mem_get_uint() works like your version. > >> > >> The same for the other get functions whose result is often ignored. > > For some functions I created a "proxy" functions in func.c the way you > > described, but not for this function since it is only used in a few places of > > sql/func.c. Should I do this for all functions? In func.c I mean. I see this as > > temporary measure, since I hope we will rework built-in functions one day. > > Unfortunately, 'hope' is not enough. And it is highly possible the code > will live for long. Therefore I think we need to make it solid where possible > and clearly state it is unsafe or add assertions where it is not possible. > > Here mem_get_uint() result is ignored always. Even if it fails. I think it > must be called something like mem_get_uint_unsafe() and return the uint as > 'return', not via an out argument. Then at least we would see it is broken > when we are around this code again, and it won't raise questions if it is a > known issue, and why it is not fixed (this must be in a comment for the > function). Understood. I created a new function, mem_get_uint_unsafe(). Diff: diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 6e6978bbc..5503a9b16 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -201,9 +201,7 @@ absFunc(sql_context * context, int argc, sql_value ** argv) UNUSED_PARAMETER(argc); switch (sql_value_type(argv[0])) { case MP_UINT: { - uint64_t u; - mem_get_uint(argv[0], &u); - sql_result_uint(context, u); + sql_result_uint(context, mem_get_uint_unsafe(argv[0])); break; } case MP_INT: { @@ -1171,7 +1169,7 @@ charFunc(sql_context * context, int argc, sql_value ** argv) if (sql_value_type(argv[i]) == MP_INT) x = 0xfffd; else - mem_get_uint(argv[i], &x); + x = mem_get_uint_unsafe(argv[i]); if (x > 0x10ffff) x = 0xfffd; c = (unsigned)(x & 0x1fffff); diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index 4aadcd3f7..ca8a75b50 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -776,6 +776,18 @@ mem_get_int_unsafe(const struct Mem *mem) int mem_get_uint(const struct Mem *mem, uint64_t *u); +/** + * Return value of MEM converted to uint64_t. This function is not safe, since + * its behaviour is undefined if mem_get_uint() returned an error. + */ +static inline uint64_t +mem_get_uint_unsafe(const struct Mem *mem) +{ + uint64_t u; + mem_get_uint(mem, &u); + return u; +} + /** * Simple type to str convertor. It is used to simplify * error reporting. New patch: commit b9ca33e93110ecf167329a0f58473371de5c7c45 Author: Mergen Imeev <imeevma@gmail.com> Date: Wed Mar 17 13:55:37 2021 +0300 sql: introduce mem_get_uint() This patch introduces mem_get_uint() function. This function is used to receive unsigned value from MEM. If value of MEM is not unsigned, it is converted to unsigned if possible. MEM is not changed. Part of #5818 diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 0db698174..5503a9b16 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -201,7 +201,7 @@ absFunc(sql_context * context, int argc, sql_value ** argv) UNUSED_PARAMETER(argc); switch (sql_value_type(argv[0])) { case MP_UINT: { - sql_result_uint(context, sql_value_uint64(argv[0])); + sql_result_uint(context, mem_get_uint_unsafe(argv[0])); break; } case MP_INT: { @@ -1169,7 +1169,7 @@ charFunc(sql_context * context, int argc, sql_value ** argv) if (sql_value_type(argv[i]) == MP_INT) x = 0xfffd; else - x = sql_value_uint64(argv[i]); + x = mem_get_uint_unsafe(argv[i]); if (x > 0x10ffff) x = 0xfffd; c = (unsigned)(x & 0x1fffff); diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index adf5e236b..ab31029df 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -1074,6 +1074,33 @@ mem_get_int(const struct Mem *mem, int64_t *i, bool *is_neg) return -1; } +int +mem_get_uint(const struct Mem *mem, uint64_t *u) +{ + if ((mem->flags & MEM_Int) != 0) + return -1; + if ((mem->flags & MEM_UInt) != 0) { + *u = mem->u.u; + return 0; + } + if ((mem->flags & (MEM_Str | MEM_Blob)) != 0) { + bool is_neg; + if (sql_atoi64(mem->z, (int64_t *)u, &is_neg, mem->n) != 0 || + is_neg) + return -1; + return 0; + } + if ((mem->flags & MEM_Real) != 0) { + double d = mem->u.r; + if (d >= 0 && d < (double)UINT64_MAX) { + *u = (uint64_t)d; + return 0; + } + return -1; + } + return -1; +} + int mem_copy(struct Mem *to, const struct Mem *from) { @@ -2283,16 +2310,6 @@ sql_value_boolean(sql_value *val) return b; } -uint64_t -sql_value_uint64(sql_value *val) -{ - int64_t i = 0; - bool is_neg; - mem_get_int((struct Mem *) val, &i, &is_neg); - assert(!is_neg); - return i; -} - const unsigned char * sql_value_text(sql_value * pVal) { diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index f3d9043e5..ca8a75b50 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -768,6 +768,26 @@ mem_get_int_unsafe(const struct Mem *mem) return i; } +/** + * Return value for MEM of UNSIGNED type. For MEM of all other types convert + * value of the MEM to UNSIGNED if possible and return converted value. Original + * MEM is not changed. + */ +int +mem_get_uint(const struct Mem *mem, uint64_t *u); + +/** + * Return value of MEM converted to uint64_t. This function is not safe, since + * its behaviour is undefined if mem_get_uint() returned an error. + */ +static inline uint64_t +mem_get_uint_unsafe(const struct Mem *mem) +{ + uint64_t u; + mem_get_uint(mem, &u); + return u; +} + /** * Simple type to str convertor. It is used to simplify * error reporting. @@ -835,9 +855,6 @@ sql_value_double(struct Mem *); bool sql_value_boolean(struct Mem *val); -uint64_t -sql_value_uint64(struct Mem *val); - const unsigned char * sql_value_text(struct Mem *); diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 0af247ebf..6ead9b261 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -448,9 +448,6 @@ sql_column_double(sql_stmt *, int iCol); bool sql_column_boolean(struct sql_stmt *stmt, int column); -uint64_t -sql_column_uint64(struct sql_stmt *stmt, int column); - const unsigned char * sql_column_text(sql_stmt *, int iCol); diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c index 5e5957496..1126425bc 100644 --- a/src/box/sql/vdbeapi.c +++ b/src/box/sql/vdbeapi.c @@ -476,12 +476,6 @@ sql_column_boolean(struct sql_stmt *stmt, int i) return sql_value_boolean(columnMem(stmt, i)); } -uint64_t -sql_column_uint64(sql_stmt * pStmt, int i) -{ - return sql_value_uint64(columnMem(pStmt, i)); -} - const unsigned char * sql_column_text(sql_stmt * pStmt, int i) {
next prev parent reply other threads:[~2021-04-14 0:39 UTC|newest] Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <cover.1618000036.git.imeevma@gmail.com> 2021-04-09 20:53 ` [Tarantool-patches] [PATCH v5 41/52] sql: introduce mem_to_number() Mergen Imeev via Tarantool-patches 2021-04-13 23:25 ` Mergen Imeev via Tarantool-patches 2021-04-09 20:53 ` [Tarantool-patches] [PATCH v5 42/52] sql: introduce mem_to_str() and mem_to_str0() Mergen Imeev via Tarantool-patches 2021-04-13 22:58 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-13 23:41 ` Mergen Imeev via Tarantool-patches 2021-04-09 20:53 ` [Tarantool-patches] [PATCH v5 43/52] sql: introduce mem_cast_explicit() Mergen Imeev via Tarantool-patches 2021-04-13 22:59 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-14 0:01 ` Mergen Imeev via Tarantool-patches 2021-04-09 20:53 ` [Tarantool-patches] [PATCH v5 44/52] sql: introduce mem_cast_implicit() Mergen Imeev via Tarantool-patches 2021-04-13 22:59 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-14 0:05 ` Mergen Imeev via Tarantool-patches 2021-04-09 20:53 ` [Tarantool-patches] [PATCH v5 45/52] sql: introduce mem_get_int() Mergen Imeev via Tarantool-patches 2021-04-13 23:01 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-14 0:28 ` Mergen Imeev via Tarantool-patches 2021-04-14 1:17 ` Mergen Imeev via Tarantool-patches 2021-04-09 21:08 ` [Tarantool-patches] [PATCH v5 46/52] sql: introduce mem_get_uint() Mergen Imeev via Tarantool-patches 2021-04-13 23:04 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-14 0:39 ` Mergen Imeev via Tarantool-patches [this message] 2021-04-14 1:21 ` Mergen Imeev via Tarantool-patches 2021-04-09 21:08 ` [Tarantool-patches] [PATCH v5 47/52] sql: introduce mem_get_double() Mergen Imeev via Tarantool-patches 2021-04-13 23:04 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-14 1:00 ` Mergen Imeev via Tarantool-patches 2021-04-15 0:17 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-15 0:46 ` Mergen Imeev via Tarantool-patches 2021-04-09 21:08 ` [Tarantool-patches] [PATCH v5 48/52] sql: introduce mem_get_bool() Mergen Imeev via Tarantool-patches 2021-04-13 23:04 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-14 1:29 ` Mergen Imeev via Tarantool-patches 2021-04-09 21:08 ` [Tarantool-patches] [PATCH v5 49/52] sql: introduce mem_get_str0() and mem_as_str0() Mergen Imeev via Tarantool-patches 2021-04-13 23:06 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-14 1:43 ` Mergen Imeev via Tarantool-patches 2021-04-09 21:08 ` [Tarantool-patches] [PATCH v5 50/52] sql: introduce mem_get_bin() Mergen Imeev via Tarantool-patches 2021-04-09 21:08 ` [Tarantool-patches] [PATCH v5 51/52] sql: introduce mem_get_bytes_len() Mergen Imeev via Tarantool-patches 2021-04-13 23:06 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-14 1:55 ` Mergen Imeev via Tarantool-patches 2021-04-15 0:21 ` Vladislav Shpilevoy via Tarantool-patches 2021-04-15 0:51 ` Mergen Imeev via Tarantool-patches 2021-04-09 21:08 ` [Tarantool-patches] [PATCH v5 52/52] sql: introduce mem_get_agg() Mergen Imeev via Tarantool-patches
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=20210414003948.GA140789@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imeevma@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v5 46/52] sql: introduce mem_get_uint()' \ /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