[tarantool-patches] Re: [PATCH v9 4/7] lua: remove exceptions from function luaL_tofield()

[Vladislav Shpilevoy]

Thanks for the fixes! I've pushed my review fixes on the
branch, and dropped at the end of the email. But I've not
tested them, so I propose you to do that, if you agree
with the concept.

=========================================================

diff --git a/src/lua/utils.c b/src/lua/utils.c
index 6150e84ff..38245a979 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -399,47 +399,58 @@ lua_field_inspect_ucdata(struct lua_State *L, struct luaL_serializer *cfg,
 }
 
 /**
- * Check that the field LUAL_SERIALIZE of the metatable is
- * available. If so, create field using it.
- *
- * This function returns two values on stack. On top of the stack
- * is boolean that shows if error happened. On the next position
- * can be boolean or result of execution of function from the
- * metatable. If it is boolean, that if its value is false than
- * there were no metatables.It its value is true than there was
- * string in that field of metatable.
- *
- * @param L Lua State.
+ * A helper structure to simplify safe call of __serialize method.
+ * It passes some arguments into the serializer called via pcall,
+ * and carries out some results.
+ */
+struct lua_serialize_status {
+	/**
+	 * True if an attempt to call __serialize has failed. A
+	 * diag message is set.
+	 */
+	bool is_error;
+	/**
+	 * True, if __serialize exists. Otherwise an ordinary
+	 * default serialization is used.
+	 */
+	bool is_serialize_used;
+	/** Parameters, passed originally to luaL_tofield. */
+	struct luaL_serializer *cfg;
+	struct luaL_field *field;
+};
+
+/**
+ * Call __serialize method of a table object if the former exists.
+ * The function expects 2 values pushed onto the Lua stack: a
+ * value to serialize, and a pointer at a struct
+ * lua_serialize_status object. If __serialize exists and is
+ * correct, then one value is pushed as a result of serialization.
+ * Otherwise is_error and is_serialize_used attributes of status
+ * should be checked.
  */
 static int
-get_metafield_serialize(struct lua_State *L)
+lua_field_try_serialize(struct lua_State *L)
 {
-	if (luaL_getmetafield(L, 1, LUAL_SERIALIZE) == 0) {
-		lua_pushboolean(L, false);
-		lua_pushboolean(L, false);
-		return 2;
-	}
-	bool is_error = false;
-	struct luaL_serializer *cfg =
-		(struct luaL_serializer *)lua_touserdata(L, 2);
-	struct luaL_field *field =
-		(struct luaL_field *)lua_touserdata(L, 3);
-
+	struct lua_serialize_status *s =
+		(struct lua_serialize_status *) lua_touserdata(L, 2);
+	s->is_serialize_used = (luaL_getmetafield(L, 1, LUAL_SERIALIZE) != 0);
+	s->is_error = false;
+	if (! s->is_serialize_used)
+		return 0;
+	struct luaL_serializer *cfg = s->cfg;
+	struct luaL_field *field = s->field;
 	if (lua_isfunction(L, -1)) {
 		/* copy object itself */
 		lua_pushvalue(L, 1);
 		lua_call(L, 1, 1);
-		is_error = (luaL_tofield(L, cfg, -1, field) != 0);
-		lua_pushboolean(L, is_error);
-		return 2;
+		s->is_error = (luaL_tofield(L, cfg, -1, field) != 0);
+		return s->is_error ? 0 : 1;
 	}
-
 	if (!lua_isstring(L, -1)) {
 		diag_set(LuajitError, "invalid " LUAL_SERIALIZE " value");
-		lua_pushboolean(L, true);
-		return 2;
+		s->is_error = true;
+		return 0;
 	}
-
 	const char *type = lua_tostring(L, -1);
 	if (strcmp(type, "array") == 0 || strcmp(type, "seq") == 0 ||
 	    strcmp(type, "sequence") == 0) {
@@ -449,9 +460,7 @@ get_metafield_serialize(struct lua_State *L)
 		if (cfg->has_compact && type[3] == '\0')
 			field->compact = true;
 		lua_pop(L, 1); /* type */
-		lua_pushboolean(L, true);
-		lua_pushboolean(L, false);
-		return 2;
+		return 1;
 	} else if (strcmp(type, "map") == 0 || strcmp(type, "mapping") == 0) {
 		field->type = MP_MAP;   /* Override type */
 		field->size = luaL_maplen(L, 1);
@@ -459,13 +468,11 @@ get_metafield_serialize(struct lua_State *L)
 		if (cfg->has_compact && type[3] == '\0')
 			field->compact = true;
 		lua_pop(L, 1); /* type */
-		lua_pushboolean(L, true);
-		lua_pushboolean(L, false);
-		return 2;
+		return 1;
 	} else {
 		diag_set(LuajitError, "invalid " LUAL_SERIALIZE " value");
-		lua_pushboolean(L, true);
-		return 2;
+		s->is_error = true;
+		return 0;
 	}
 }
 
@@ -478,27 +485,19 @@ lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
 	uint32_t max = 0;
 
 	if (cfg->encode_load_metatables) {
-		lua_pushcfunction(L, get_metafield_serialize);
+		struct lua_serialize_status s;
+		s.cfg = cfg;
+		s.field = field;
+		lua_pushcfunction(L, lua_field_try_serialize);
 		lua_pushvalue(L, idx);
-		lua_pushlightuserdata(L, cfg);
-		lua_pushlightuserdata(L, field);
-		if (lua_pcall(L, 3, 2, 0) != 0) {
+		lua_pushlightuserdata(L, &s);
+		if (lua_pcall(L, 2, 1, 0) != 0) {
 			diag_set(LuajitError, lua_tostring(L, -1));
 			return -1;
 		}
-		bool is_error = lua_toboolean(L, -1);
-		bool is_bool = lua_isboolean(L, -2);
-		if (is_error) {
-			lua_pop(L, 2);
+		if (s.is_error)
 			return -1;
-		}
-		if (is_bool) {
-			bool is_metatable_available = lua_toboolean(L, -2);
-			lua_pop(L, 2);
-			if (is_metatable_available)
-				return 0;
-		} else {
-			lua_pop(L, 1);
+		if (s.is_serialize_used) {
 			lua_replace(L, idx);
 			return 0;
 		}