From 85641c545bedbdb703ad923306786afe5c312110 Mon Sep 17 00:00:00 2001 From: Hein-Pieter van Braam Date: Mon, 18 Sep 2017 13:09:06 +0200 Subject: [PATCH 1/2] Be type-strict checking on equality checks After a short discussion with @reduz and @karroffel we decided to make all non number/number comparisons return type errors on comparisons. Now bool == bool is allowed but Vector2 == Vector3 is a type error and no longer 'not equal'. The same has been done for the != operators. In addition I forgot to add some failures to some Object operators meaning that there was a potential for a crasher. --- core/variant_op.cpp | 130 +++++++++++++++++++++----------------------- 1 file changed, 63 insertions(+), 67 deletions(-) diff --git a/core/variant_op.cpp b/core/variant_op.cpp index 142919337d3..8349f2fdf6f 100644 --- a/core/variant_op.cpp +++ b/core/variant_op.cpp @@ -421,11 +421,12 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, if (p_b.type == NIL) _RETURN(true); if (p_b.type == OBJECT) _RETURN(p_b._get_obj().obj == NULL); - _RETURN(false); + _RETURN_FAIL; } CASE_TYPE(math, OP_EQUAL, BOOL) { - if (p_b.type != BOOL) _RETURN(false); + if (p_b.type != BOOL) + _RETURN_FAIL; _RETURN(p_a._data._bool == p_b._data._bool); } @@ -434,11 +435,12 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, _RETURN((p_a._get_obj().obj == p_b._get_obj().obj)); if (p_b.type == NIL) _RETURN(p_a._get_obj().obj == NULL); + _RETURN_FAIL; } CASE_TYPE(math, OP_EQUAL, DICTIONARY) { if (p_b.type != DICTIONARY) - _RETURN(false); + _RETURN_FAIL; const Dictionary *arr_a = reinterpret_cast(p_a._data._mem); const Dictionary *arr_b = reinterpret_cast(p_b._data._mem); @@ -448,7 +450,7 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, CASE_TYPE(math, OP_EQUAL, ARRAY) { if (p_b.type != ARRAY) - _RETURN(false); + _RETURN_FAIL; const Array *arr_a = reinterpret_cast(p_a._data._mem); const Array *arr_b = reinterpret_cast(p_b._data._mem); @@ -495,11 +497,12 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, if (p_b.type == NIL) _RETURN(false); if (p_b.type == OBJECT) _RETURN(p_b._get_obj().obj != NULL); - _RETURN(true); + _RETURN_FAIL; } CASE_TYPE(math, OP_NOT_EQUAL, BOOL) { - if (p_b.type != BOOL) _RETURN(true); + if (p_b.type != BOOL) + _RETURN_FAIL; _RETURN(p_a._data._bool != p_b._data._bool); } @@ -508,11 +511,12 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, _RETURN((p_a._get_obj().obj != p_b._get_obj().obj)); if (p_b.type == NIL) _RETURN(p_a._get_obj().obj != NULL); + _RETURN_FAIL; } CASE_TYPE(math, OP_NOT_EQUAL, DICTIONARY) { if (p_b.type != DICTIONARY) - _RETURN(true); + _RETURN_FAIL; const Dictionary *arr_a = reinterpret_cast(p_a._data._mem); const Dictionary *arr_b = reinterpret_cast(p_b._data._mem); @@ -522,7 +526,7 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, CASE_TYPE(math, OP_NOT_EQUAL, ARRAY) { if (p_b.type != ARRAY) - _RETURN(true); + _RETURN_FAIL; const Array *arr_a = reinterpret_cast(p_a._data._mem); const Array *arr_b = reinterpret_cast(p_b._data._mem); @@ -580,13 +584,14 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, } CASE_TYPE(math, OP_LESS, OBJECT) { - if (p_b.type == OBJECT) - _RETURN((p_a._get_obj().obj < p_b._get_obj().obj)); + if (p_b.type != OBJECT) + _RETURN_FAIL; + _RETURN((p_a._get_obj().obj < p_b._get_obj().obj)); } CASE_TYPE(math, OP_LESS, ARRAY) { if (p_b.type != ARRAY) - _RETURN(false); + _RETURN_FAIL; const Array *arr_a = reinterpret_cast(p_a._data._mem); const Array *arr_b = reinterpret_cast(p_b._data._mem); @@ -633,8 +638,9 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, SWITCH_OP(math, OP_LESS_EQUAL, p_a.type) { CASE_TYPE(math, OP_LESS_EQUAL, OBJECT) { - if (p_b.type == OBJECT) - _RETURN((p_a._get_obj().obj <= p_b._get_obj().obj)); + if (p_b.type != OBJECT) + _RETURN_FAIL; + _RETURN((p_a._get_obj().obj <= p_b._get_obj().obj)); } DEFAULT_OP_NUM(math, OP_LESS_EQUAL, INT, <=, _int); @@ -682,13 +688,14 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, } CASE_TYPE(math, OP_GREATER, OBJECT) { - if (p_b.type == OBJECT) - _RETURN((p_a._get_obj().obj > p_b._get_obj().obj)); + if (p_b.type != OBJECT) + _RETURN_FAIL; + _RETURN((p_a._get_obj().obj > p_b._get_obj().obj)); } CASE_TYPE(math, OP_GREATER, ARRAY) { if (p_b.type != ARRAY) - _RETURN(false); + _RETURN_FAIL; const Array *arr_a = reinterpret_cast(p_a._data._mem); const Array *arr_b = reinterpret_cast(p_b._data._mem); @@ -735,8 +742,9 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, SWITCH_OP(math, OP_GREATER_EQUAL, p_a.type) { CASE_TYPE(math, OP_GREATER_EQUAL, OBJECT) { - if (p_b.type == OBJECT) - _RETURN((p_a._get_obj().obj >= p_b._get_obj().obj)); + if (p_b.type != OBJECT) + _RETURN_FAIL; + _RETURN((p_a._get_obj().obj >= p_b._get_obj().obj)); } DEFAULT_OP_NUM(math, OP_GREATER_EQUAL, INT, >=, _int); @@ -771,10 +779,9 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, SWITCH_OP(math, OP_ADD, p_a.type) { CASE_TYPE(math, OP_ADD, ARRAY) { - if (p_a.type != p_b.type) { - r_valid = false; - return; - } + if (p_a.type != p_b.type) + _RETURN_FAIL; + const Array &array_a = *reinterpret_cast(p_a._data._mem); const Array &array_b = *reinterpret_cast(p_b._data._mem); Array sum; @@ -853,65 +860,54 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, SWITCH_OP(math, OP_MULTIPLY, p_a.type) { CASE_TYPE(math, OP_MULTIPLY, TRANSFORM2D) { - if (p_b.type == TRANSFORM2D) { - _RETURN(*p_a._data._transform2d * *p_b._data._transform2d); - }; - if (p_b.type == VECTOR2) { - _RETURN(p_a._data._transform2d->xform(*(const Vector2 *)p_b._data._mem)); - }; - r_valid = false; - return; + switch (p_b.type) { + case TRANSFORM2D: { + _RETURN(*p_a._data._transform2d * *p_b._data._transform2d); + } + case VECTOR2: { + _RETURN(p_a._data._transform2d->xform(*(const Vector2 *)p_b._data._mem)); + } + default: _RETURN_FAIL; + } } CASE_TYPE(math, OP_MULTIPLY, QUAT) { switch (p_b.type) { case VECTOR3: { - _RETURN(reinterpret_cast(p_a._data._mem)->xform(*(const Vector3 *)p_b._data._mem)); - } break; + } case QUAT: { - _RETURN(*reinterpret_cast(p_a._data._mem) * *reinterpret_cast(p_b._data._mem)); - } break; + } case REAL: { _RETURN(*reinterpret_cast(p_a._data._mem) * p_b._data._real); - } break; - default: {} - }; - r_valid = false; - return; + } + default: _RETURN_FAIL; + } } CASE_TYPE(math, OP_MULTIPLY, BASIS) { switch (p_b.type) { case VECTOR3: { - _RETURN(p_a._data._basis->xform(*(const Vector3 *)p_b._data._mem)); - }; + } case BASIS: { - _RETURN(*p_a._data._basis * *p_b._data._basis); - }; - default: {} - }; - r_valid = false; - return; + } + default: _RETURN_FAIL; + } } CASE_TYPE(math, OP_MULTIPLY, TRANSFORM) { switch (p_b.type) { case VECTOR3: { - _RETURN(p_a._data._transform->xform(*(const Vector3 *)p_b._data._mem)); - }; + } case TRANSFORM: { - _RETURN(*p_a._data._transform * *p_b._data._transform); - }; - default: {} - }; - r_valid = false; - return; + } + default: _RETURN_FAIL; + } } DEFAULT_OP_NUM_VEC(math, OP_MULTIPLY, INT, *, _int); @@ -943,18 +939,15 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, SWITCH_OP(math, OP_DIVIDE, p_a.type) { CASE_TYPE(math, OP_DIVIDE, QUAT) { - if (p_b.type != REAL) { - r_valid = false; - return; - } + if (p_b.type != REAL) + _RETURN_FAIL; #ifdef DEBUG_ENABLED if (p_b._data._real == 0) { r_valid = false; _RETURN("Division By Zero"); } #endif - _RETURN( - *reinterpret_cast(p_a._data._mem) / p_b._data._real); + _RETURN(*reinterpret_cast(p_a._data._mem) / p_b._data._real); } DEFAULT_OP_NUM_DIV(math, OP_DIVIDE, INT, _int); @@ -1054,9 +1047,8 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, SWITCH_OP(math, OP_MODULE, p_a.type) { CASE_TYPE(math, OP_MODULE, INT) { - if (p_b.type != INT) { + if (p_b.type != INT) _RETURN_FAIL; - } #ifdef DEBUG_ENABLED if (p_b._data._int == 0) { r_valid = false; @@ -1067,15 +1059,13 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, } CASE_TYPE(math, OP_MODULE, STRING) { - const String *format = - reinterpret_cast(p_a._data._mem); + const String *format = reinterpret_cast(p_a._data._mem); String result; bool error; if (p_b.type == ARRAY) { // e.g. "frog %s %d" % ["fish", 12] - const Array *args = - reinterpret_cast(p_b._data._mem); + const Array *args = reinterpret_cast(p_b._data._mem); result = format->sprintf(*args, &error); } else { // e.g. "frog %d" % 12 @@ -1127,6 +1117,7 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, _RETURN_FAIL; _RETURN(p_a._data._int << p_b._data._int); } + CASE_TYPE_ALL_BUT_INT(math, OP_SHIFT_LEFT) _RETURN_FAIL; } @@ -1137,6 +1128,7 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, _RETURN_FAIL; _RETURN(p_a._data._int >> p_b._data._int); } + CASE_TYPE_ALL_BUT_INT(math, OP_SHIFT_RIGHT) _RETURN_FAIL; } @@ -1147,6 +1139,7 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, _RETURN_FAIL; _RETURN(p_a._data._int & p_b._data._int); } + CASE_TYPE_ALL_BUT_INT(math, OP_BIT_AND) _RETURN_FAIL; } @@ -1157,6 +1150,7 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, _RETURN_FAIL; _RETURN(p_a._data._int | p_b._data._int); } + CASE_TYPE_ALL_BUT_INT(math, OP_BIT_OR) _RETURN_FAIL; } @@ -1167,6 +1161,7 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, _RETURN_FAIL; _RETURN(p_a._data._int ^ p_b._data._int); } + CASE_TYPE_ALL_BUT_INT(math, OP_BIT_XOR) _RETURN_FAIL; } @@ -1175,6 +1170,7 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, CASE_TYPE(math, OP_BIT_NEGATE, INT) { _RETURN(~p_a._data._int); } + CASE_TYPE_ALL_BUT_INT(math, OP_BIT_NEGATE) _RETURN_FAIL; } From 833c3917b247baa46f5a5f6ad6ce478cffc1911d Mon Sep 17 00:00:00 2001 From: Hein-Pieter van Braam Date: Mon, 18 Sep 2017 20:02:47 +0200 Subject: [PATCH 2/2] Allow booleanization of all types We now allow booleanization of all types. This means that empty versions of all types now evaluate to false. So a Vector2(0,0), Dictionary(), etc. This allows you to write GDScript like: if not Dictionary(): print("Empty dict") Booleanization can now also no longer fail. There is no more valid flag, this changes Variant and GDNative API. --- core/variant.h | 2 +- core/variant_op.cpp | 87 +++---------------- modules/gdnative/gdnative/variant.cpp | 5 +- modules/gdnative/include/gdnative/variant.h | 2 +- .../gdnative/include/gdnative_api_struct.h | 2 +- modules/gdscript/gd_function.cpp | 27 +----- 6 files changed, 20 insertions(+), 105 deletions(-) diff --git a/core/variant.h b/core/variant.h index edc86dedd40..5ea540a63fe 100644 --- a/core/variant.h +++ b/core/variant.h @@ -390,7 +390,7 @@ public: uint32_t hash() const; bool hash_compare(const Variant &p_variant) const; - bool booleanize(bool &valid) const; + bool booleanize() const; void static_assign(const Variant &p_variant); static void get_constructor_list(Variant::Type p_type, List *p_list); diff --git a/core/variant_op.cpp b/core/variant_op.cpp index 8349f2fdf6f..5d47936679f 100644 --- a/core/variant_op.cpp +++ b/core/variant_op.cpp @@ -143,56 +143,13 @@ Variant::operator bool() const { - bool b; - return booleanize(b); + return booleanize(); } -bool Variant::booleanize(bool &r_valid) const { - - r_valid = true; - switch (type) { - case NIL: - return false; - case BOOL: - return _data._bool; - case INT: - return _data._int; - case REAL: - return _data._real; - case STRING: - return (*reinterpret_cast(_data._mem)) != ""; - case VECTOR2: - case RECT2: - case TRANSFORM2D: - case VECTOR3: - case PLANE: - case RECT3: - case QUAT: - case BASIS: - case TRANSFORM: - case COLOR: - case _RID: - return (*reinterpret_cast(_data._mem)).is_valid(); - case OBJECT: - return _get_obj().obj; - case NODE_PATH: - return (*reinterpret_cast(_data._mem)) != NodePath(); - case DICTIONARY: - case ARRAY: - case POOL_BYTE_ARRAY: - case POOL_INT_ARRAY: - case POOL_REAL_ARRAY: - case POOL_STRING_ARRAY: - case POOL_VECTOR2_ARRAY: - case POOL_VECTOR3_ARRAY: - case POOL_COLOR_ARRAY: - r_valid = false; - return false; - default: { - } - } - - return false; +// We consider all unitialized or empty types to be false based on the type's +// zeroiness. +bool Variant::booleanize() const { + return !is_zero(); } #define _RETURN(m_what) \ @@ -403,12 +360,6 @@ bool Variant::booleanize(bool &r_valid) const { _RETURN(sum); \ } -#define DEFAULT_OP_FAIL(m_name) \ - case m_name: { \ - r_valid = false; \ - return; \ - } - void Variant::evaluate(const Operator &p_op, const Variant &p_a, const Variant &p_b, Variant &r_ret, bool &r_valid) { @@ -1177,12 +1128,8 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, SWITCH_OP(math, OP_AND, p_a.type) { CASE_TYPE_ALL(math, OP_AND) { - bool l = p_a.booleanize(r_valid); - if (!r_valid) - return; - bool r = p_b.booleanize(r_valid); - if (!r_valid) - return; + bool l = p_a.booleanize(); + bool r = p_b.booleanize(); _RETURN(l && r); } @@ -1190,12 +1137,8 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, SWITCH_OP(math, OP_OR, p_a.type) { CASE_TYPE_ALL(math, OP_OR) { - bool l = p_a.booleanize(r_valid); - if (!r_valid) - return; - bool r = p_b.booleanize(r_valid); - if (!r_valid) - return; + bool l = p_a.booleanize(); + bool r = p_b.booleanize(); _RETURN(l || r); } @@ -1203,12 +1146,8 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, SWITCH_OP(math, OP_XOR, p_a.type) { CASE_TYPE_ALL(math, OP_XOR) { - bool l = p_a.booleanize(r_valid); - if (!r_valid) - return; - bool r = p_b.booleanize(r_valid); - if (!r_valid) - return; + bool l = p_a.booleanize(); + bool r = p_b.booleanize(); _RETURN((l || r) && !(l && r)); } @@ -1216,9 +1155,7 @@ void Variant::evaluate(const Operator &p_op, const Variant &p_a, SWITCH_OP(math, OP_NOT, p_a.type) { CASE_TYPE_ALL(math, OP_NOT) { - bool l = p_a.booleanize(r_valid); - if (!r_valid) - return; + bool l = p_a.booleanize(); _RETURN(!l); } } diff --git a/modules/gdnative/gdnative/variant.cpp b/modules/gdnative/gdnative/variant.cpp index 1b2aae607fd..9ba4166c1d3 100644 --- a/modules/gdnative/gdnative/variant.cpp +++ b/modules/gdnative/gdnative/variant.cpp @@ -480,10 +480,9 @@ godot_bool GDAPI godot_variant_hash_compare(const godot_variant *p_self, const g return self->hash_compare(*other); } -godot_bool GDAPI godot_variant_booleanize(const godot_variant *p_self, godot_bool *r_valid) { +godot_bool GDAPI godot_variant_booleanize(const godot_variant *p_self) { const Variant *self = (const Variant *)p_self; - bool &valid = *r_valid; - return self->booleanize(valid); + return self->booleanize(); } void GDAPI godot_variant_destroy(godot_variant *p_self) { diff --git a/modules/gdnative/include/gdnative/variant.h b/modules/gdnative/include/gdnative/variant.h index 969506585dd..7b804c1eaf0 100644 --- a/modules/gdnative/include/gdnative/variant.h +++ b/modules/gdnative/include/gdnative/variant.h @@ -190,7 +190,7 @@ godot_bool GDAPI godot_variant_operator_less(const godot_variant *p_self, const godot_bool GDAPI godot_variant_hash_compare(const godot_variant *p_self, const godot_variant *p_other); -godot_bool GDAPI godot_variant_booleanize(const godot_variant *p_self, godot_bool *r_valid); +godot_bool GDAPI godot_variant_booleanize(const godot_variant *p_self); void GDAPI godot_variant_destroy(godot_variant *p_self); diff --git a/modules/gdnative/include/gdnative_api_struct.h b/modules/gdnative/include/gdnative_api_struct.h index cfebeb08cc2..c345e272273 100644 --- a/modules/gdnative/include/gdnative_api_struct.h +++ b/modules/gdnative/include/gdnative_api_struct.h @@ -534,7 +534,7 @@ extern "C" { GDAPI_FUNC(godot_variant_operator_equal, godot_bool, const godot_variant *p_self, const godot_variant *p_other) \ GDAPI_FUNC(godot_variant_operator_less, godot_bool, const godot_variant *p_self, const godot_variant *p_other) \ GDAPI_FUNC(godot_variant_hash_compare, godot_bool, const godot_variant *p_self, const godot_variant *p_other) \ - GDAPI_FUNC(godot_variant_booleanize, godot_bool, const godot_variant *p_self, godot_bool *r_valid) \ + GDAPI_FUNC(godot_variant_booleanize, godot_bool, const godot_variant *p_self) \ GDAPI_FUNC_VOID(godot_variant_destroy, godot_variant *p_self) \ GDAPI_FUNC_VOID(godot_string_new, godot_string *r_dest) \ GDAPI_FUNC_VOID(godot_string_new_copy, godot_string *r_dest, const godot_string *p_src) \ diff --git a/modules/gdscript/gd_function.cpp b/modules/gdscript/gd_function.cpp index 438d484a414..df7b16c96ec 100644 --- a/modules/gdscript/gd_function.cpp +++ b/modules/gdscript/gd_function.cpp @@ -982,15 +982,8 @@ Variant GDFunction::call(GDInstance *p_instance, const Variant **p_args, int p_a GET_VARIANT_PTR(test, 1); - bool valid; - bool result = test->booleanize(valid); -#ifdef DEBUG_ENABLED - if (!valid) { + bool result = test->booleanize(); - err_text = "cannot evaluate conditional expression of type: " + Variant::get_type_name(test->get_type()); - break; - } -#endif if (result) { int to = _code_ptr[ip + 2]; GD_ERR_BREAK(to < 0 || to > _code_size); @@ -1006,15 +999,8 @@ Variant GDFunction::call(GDInstance *p_instance, const Variant **p_args, int p_a GET_VARIANT_PTR(test, 1); - bool valid; - bool result = test->booleanize(valid); -#ifdef DEBUG_ENABLED - if (!valid) { + bool result = test->booleanize(); - err_text = "cannot evaluate conditional expression of type: " + Variant::get_type_name(test->get_type()); - break; - } -#endif if (!result) { int to = _code_ptr[ip + 2]; GD_ERR_BREAK(to < 0 || to > _code_size); @@ -1107,14 +1093,7 @@ Variant GDFunction::call(GDInstance *p_instance, const Variant **p_args, int p_a GET_VARIANT_PTR(test, 1); #ifdef DEBUG_ENABLED - bool valid; - bool result = test->booleanize(valid); - - if (!valid) { - - err_text = "cannot evaluate conditional expression of type: " + Variant::get_type_name(test->get_type()); - break; - } + bool result = test->booleanize(); if (!result) {