[tarantool-patches] Re: [PATCH 7/8] sql: clean-up affinity from SQL source code
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Feb 5 18:08:16 MSK 2019
Thanks for the fixes!
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 358b69754..dae05e80f 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2153,13 +2153,13 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */
> } else {
> /*
> * Neither operand is NULL. Do a comparison.
'Neither operand is NULL' - this is 'else' branch of
"(flags1 | flags3)&MEM_Null", so I guess it is obvious.
> - * 15 is 1111 in a binary form.
> - * Since all existing types can be fitted in 4 bits
> - * (field_type_MAX == 10), it is enough to 'and'
> - * p5 with this constant. Node that all other flags
> - * that can be stored in p5 are >= 16.
> + * Since all existing types can be fitted in 4
> + * bits (field_type_MAX == 9), it is enough to
> + * 'and' p5 with field mask. Note that values of
> + * other flags that can be stored in p5 of these
> + * opcodes are >= FIELD_TYPE_MASK.
They can not be == FIELD_TYPE_MASK. Please, again, do not try to
write here any constants, including bit counts. A definition named
MASK is already enough to make it clear, that p5 'and' mask give a
type.
> */
> - enum field_type type = pOp->p5 & 15;
> + enum field_type type = pOp->p5 & FIELD_TYPE_MASK;
>
Please, apply the diff below. I made enum field_type_mask an
anonymous enum, as we usually do with independent constants. Also, I
used FIELD_TYPE_MASK instead of 15 in static assertion, and removed
the comment from vdbe.c about what a mask is.
diff --git a/src/box/field_def.h b/src/box/field_def.h
index 3b8eb0ccf..3eda0e38f 100644
--- a/src/box/field_def.h
+++ b/src/box/field_def.h
@@ -87,21 +87,22 @@ affinity_type_str(enum affinity_type type);
/** \endcond public */
+enum {
+ /**
+ * This mask allows to store in VdbeOp.p5 operand of
+ * OP_Eq, OP_Lt etc opcodes field type alonside with
+ * flags.
+ */
+ FIELD_TYPE_MASK = 15
+};
+
/**
* For detailed explanation see context of OP_Eq, OP_Lt etc
* opcodes in vdbe.c.
*/
-static_assert(field_type_MAX <= 15, "values of enum field_type "\
+static_assert(field_type_MAX <= FIELD_TYPE_MASK, "values of enum field_type "\
"should fit into 4 bits of VdbeOp.p5");
-/**
- * This mask allows to store in p5 operand of OP_Eq, OP_Lt etc
- * opcodes field type alonside with other flags.
- */
-enum field_type_mask {
- FIELD_TYPE_MASK = 15
-};
-
extern const char *field_type_strs[];
extern const char *on_conflict_action_strs[];
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index dae05e80f..a66ab4992 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2151,14 +2151,6 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */
break;
}
} else {
- /*
- * Neither operand is NULL. Do a comparison.
- * Since all existing types can be fitted in 4
- * bits (field_type_MAX == 9), it is enough to
- * 'and' p5 with field mask. Note that values of
- * other flags that can be stored in p5 of these
- * opcodes are >= FIELD_TYPE_MASK.
- */
enum field_type type = pOp->p5 & FIELD_TYPE_MASK;
if (sql_type_is_numeric(type)) {
if ((flags1 | flags3)&MEM_Str) {
More information about the Tarantool-patches
mailing list