[Tarantool-patches] [PATCH v5 46/52] sql: introduce mem_get_uint()
Mergen Imeev
imeevma at tarantool.org
Wed Apr 14 03:39:48 MSK 2021
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 at 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)
{
More information about the Tarantool-patches
mailing list