[Tarantool-patches] [PATCH v5 38/52] sql: move MEM flags to mem.c
imeevma at tarantool.org
imeevma at tarantool.org
Fri Apr 9 23:25:55 MSK 2021
Thank you for the review! My answers and new patch below.
On 30.03.2021 02:07, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
>> diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
>> index 70224b55a..8b6f6749d 100644
>> --- a/src/box/sql/mem.h
>> +++ b/src/box/sql/mem.h
>> @@ -563,13 +511,6 @@ columnNullValue(void);
>>
>> int sqlVdbeMemTooBig(Mem *);
>>
>> -/* Return TRUE if Mem X contains dynamically allocated content - anything
>> - * that needs to be deallocated to avoid a leak.
>> - */
>> -#define VdbeMemDynamic(X) \
>> - (((X)->flags&(MEM_Agg|MEM_Dyn|MEM_Frame))!=0)
>
> Why did you remove that? And why don't you have MEM_Agg|MEM_Frame in
> mem_is_dynamic()?
Function mem_is_dynamic() shows allocation type of MEM contains STRING or
VARBINARY. Macro VdbeMemDynamic() shows that MEM should be cleared before
changing. This macro is not needed now outside of mem.c since all settings there
should be done using mem_set_*() functions, which clear MEM before changing.
In mem.c this macro may be added in case we set MEM directly, however we inline
mem_set_*() only in cases when we know that there is no clearing needed. Not
sure that this macro is needed after all.
There is one place where MEM is set without mem_set_*() outside of mem.c - in
function allocateCursor(). However, the way MEM is used here is so different
from normal way of using MEM, that I am not sure that VdbeMemDynamic() is
actually proper there.
New patch:
commit 670c9835e020f49a67ee8ade81fbf59dd0062beb
Author: Mergen Imeev <imeevma at gmail.com>
Date: Wed Mar 17 10:19:57 2021 +0300
sql: move MEM flags to mem.c
This patch moves MEM flags to mem.c. This allow us to have more control
over MEM state.
Part of #5818
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 2e147291f..52b1891aa 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -52,6 +52,57 @@
static int
sqlVdbeMemGrow(struct Mem *pMem, int n, int preserve);
+/* One or more of the following flags are set to indicate the validOK
+ * representations of the value stored in the Mem struct.
+ *
+ * If the MEM_Null flag is set, then the value is an SQL NULL value.
+ * No other flags may be set in this case.
+ *
+ * If the MEM_Str flag is set then Mem.z points at a string representation.
+ * Usually this is encoded in the same unicode encoding as the main
+ * database (see below for exceptions). If the MEM_Term flag is also
+ * set, then the string is nul terminated. The MEM_Int and MEM_Real
+ * flags may coexist with the MEM_Str flag.
+ */
+#define MEM_Null 0x0001 /* Value is NULL */
+#define MEM_Str 0x0002 /* Value is a string */
+#define MEM_Int 0x0004 /* Value is an integer */
+#define MEM_Real 0x0008 /* Value is a real number */
+#define MEM_Blob 0x0010 /* Value is a BLOB */
+#define MEM_Bool 0x0020 /* Value is a bool */
+#define MEM_UInt 0x0040 /* Value is an unsigned integer */
+#define MEM_Frame 0x0080 /* Value is a VdbeFrame object */
+#define MEM_Undefined 0x0100 /* Value is undefined */
+#define MEM_Cleared 0x0200 /* NULL set by OP_Null, not from data */
+#define MEM_TypeMask 0x83ff /* Mask of type bits */
+
+/* Whenever Mem contains a valid string or blob representation, one of
+ * the following flags must be set to determine the memory management
+ * policy for Mem.z. The MEM_Term flag tells us whether or not the
+ * string is \000 or \u0000 terminated
+ */
+#define MEM_Term 0x0400 /* String rep is nul terminated */
+#define MEM_Dyn 0x0800 /* Need to call Mem.xDel() on Mem.z */
+#define MEM_Static 0x1000 /* Mem.z points to a static string */
+#define MEM_Ephem 0x2000 /* Mem.z points to an ephemeral string */
+#define MEM_Agg 0x4000 /* Mem.z points to an agg function context */
+#define MEM_Zero 0x8000 /* Mem.i contains count of 0s appended to blob */
+#define MEM_Subtype 0x10000 /* Mem.eSubtype is valid */
+#define MEM_Ptr 0x20000 /* Value is a generic pointer */
+
+/**
+ * In contrast to Mem_TypeMask, this one allows to get
+ * pure type of memory cell, i.e. without _Dyn/_Zero and other
+ * auxiliary flags.
+ */
+enum {
+ MEM_PURE_TYPE_MASK = 0x7f
+};
+
+static_assert(MEM_PURE_TYPE_MASK == (MEM_Null | MEM_Str | MEM_Int | MEM_Real |
+ MEM_Blob | MEM_Bool | MEM_UInt),
+ "value of type mask must consist of corresponding to memory "\
+ "type bits");
bool
mem_is_null(const struct Mem *mem)
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index f17c4bb78..ce5076361 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -473,58 +473,6 @@ int
mem_compare(const struct Mem *left, const struct Mem *right, int *result,
enum field_type type, struct coll *coll);
-/* One or more of the following flags are set to indicate the validOK
- * representations of the value stored in the Mem struct.
- *
- * If the MEM_Null flag is set, then the value is an SQL NULL value.
- * No other flags may be set in this case.
- *
- * If the MEM_Str flag is set then Mem.z points at a string representation.
- * Usually this is encoded in the same unicode encoding as the main
- * database (see below for exceptions). If the MEM_Term flag is also
- * set, then the string is nul terminated. The MEM_Int and MEM_Real
- * flags may coexist with the MEM_Str flag.
- */
-#define MEM_Null 0x0001 /* Value is NULL */
-#define MEM_Str 0x0002 /* Value is a string */
-#define MEM_Int 0x0004 /* Value is an integer */
-#define MEM_Real 0x0008 /* Value is a real number */
-#define MEM_Blob 0x0010 /* Value is a BLOB */
-#define MEM_Bool 0x0020 /* Value is a bool */
-#define MEM_UInt 0x0040 /* Value is an unsigned integer */
-#define MEM_Frame 0x0080 /* Value is a VdbeFrame object */
-#define MEM_Undefined 0x0100 /* Value is undefined */
-#define MEM_Cleared 0x0200 /* NULL set by OP_Null, not from data */
-#define MEM_TypeMask 0x83ff /* Mask of type bits */
-
-/* Whenever Mem contains a valid string or blob representation, one of
- * the following flags must be set to determine the memory management
- * policy for Mem.z. The MEM_Term flag tells us whether or not the
- * string is \000 or \u0000 terminated
- */
-#define MEM_Term 0x0400 /* String rep is nul terminated */
-#define MEM_Dyn 0x0800 /* Need to call Mem.xDel() on Mem.z */
-#define MEM_Static 0x1000 /* Mem.z points to a static string */
-#define MEM_Ephem 0x2000 /* Mem.z points to an ephemeral string */
-#define MEM_Agg 0x4000 /* Mem.z points to an agg function context */
-#define MEM_Zero 0x8000 /* Mem.i contains count of 0s appended to blob */
-#define MEM_Subtype 0x10000 /* Mem.eSubtype is valid */
-#define MEM_Ptr 0x20000 /* Value is a generic pointer */
-
-/**
- * In contrast to Mem_TypeMask, this one allows to get
- * pure type of memory cell, i.e. without _Dyn/_Zero and other
- * auxiliary flags.
- */
-enum {
- MEM_PURE_TYPE_MASK = 0x7f
-};
-
-static_assert(MEM_PURE_TYPE_MASK == (MEM_Null | MEM_Str | MEM_Int | MEM_Real |
- MEM_Blob | MEM_Bool | MEM_UInt),
- "value of type mask must consist of corresponding to memory "\
- "type bits");
-
/**
* Simple type to str convertor. It is used to simplify
* error reporting.
@@ -555,7 +503,7 @@ registerTrace(int iReg, Mem *p);
* Return true if a memory cell is not marked as invalid. This macro
* is for use inside assert() statements only.
*/
-#define memIsValid(M) ((M)->flags & MEM_Undefined)==0
+#define memIsValid(M) !mem_is_invalid(M)
#endif
/**
@@ -589,7 +537,7 @@ int mem_apply_integer_type(struct Mem *);
int sqlVdbeMemStringify(struct Mem *);
int sqlVdbeMemNulTerminate(struct Mem *);
int sqlVdbeMemExpandBlob(struct Mem *);
-#define ExpandBlob(P) (((P)->flags&MEM_Zero)?sqlVdbeMemExpandBlob(P):0)
+#define ExpandBlob(P) (mem_is_zerobin(P)? sqlVdbeMemExpandBlob(P) : 0)
void sql_value_apply_type(struct Mem *val, enum field_type type);
@@ -700,13 +648,6 @@ columnNullValue(void);
int sqlVdbeMemTooBig(Mem *);
-/* Return TRUE if Mem X contains dynamically allocated content - anything
- * that needs to be deallocated to avoid a leak.
- */
-#define VdbeMemDynamic(X) \
- (((X)->flags&(MEM_Agg|MEM_Dyn|MEM_Frame))!=0)
-
-
int sqlMemCompare(const Mem *, const Mem *, const struct coll *);
/**
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 4566606d7..71a827034 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -607,7 +607,6 @@ case OP_SetDiag: { /* jump */
case OP_Gosub: { /* jump */
assert(pOp->p1>0 && pOp->p1<=(p->nMem+1 - p->nCursor));
pIn1 = &aMem[pOp->p1];
- assert(VdbeMemDynamic(pIn1)==0);
memAboutToChange(p, pIn1);
mem_set_uint(pIn1, pOp - aOp);
REGISTER_TRACE(p, pOp->p1, pIn1);
@@ -649,7 +648,6 @@ case OP_InitCoroutine: { /* jump */
assert(pOp->p2>=0 && pOp->p2<p->nOp);
assert(pOp->p3>0 && pOp->p3<p->nOp);
pOut = &aMem[pOp->p1];
- assert(!VdbeMemDynamic(pOut));
mem_set_uint(pOut, pOp->p3 - 1);
if (pOp->p2) goto jump_to_p2;
break;
@@ -691,7 +689,6 @@ case OP_EndCoroutine: { /* in1 */
*/
case OP_Yield: { /* in1, jump */
pIn1 = &aMem[pOp->p1];
- assert(VdbeMemDynamic(pIn1)==0);
int pcDest = (int)pIn1->u.u;
mem_set_uint(pIn1, pOp - aOp);
REGISTER_TRACE(p, pOp->p1, pIn1);
More information about the Tarantool-patches
mailing list