[tarantool-patches] Re: [PATCH] json: added options to json.encode()

[roman.habibov1@yandex.ru]

> 1. Please, use imperative in commit header.

Fixed.


>>  diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c
>>  index aa8217d..bdfda1e 100644
>>  --- a/third_party/lua-cjson/lua_cjson.c
>>  +++ b/third_party/lua-cjson/lua_cjson.c
>>  @@ -417,23 +417,89 @@ static void json_append_data(lua_State *l, struct luaL_serializer *cfg,
>>        }
>>    }
>>
>>  -static int json_encode(lua_State *l)
>>  -{
>>  - struct luaL_serializer *cfg = luaL_checkserializer(l);
>>  - char *json;
>>  - int len;
>>  +#define OPTION(type, name, defvalue) { #name, \
>>  + offsetof(struct luaL_serializer, name), type, defvalue}
>
> 3. All this code including parse_options is a full copy of
> the code from src/lua/util.c. Please, find a way how to do not
> duplicate the code. You should consolidate an options parser
> function from util.c code and make a declaration in a header.
> And then use this function both in util.c in luaL_serializer_cfg()
> and in json_encode().
>
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 2f0f4dcf8..0626bf76b 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -186,34 +186,6 @@ luaL_setcdatagc(struct lua_State *L, int idx)
 	lua_pop(L, 1);
 }
 
-
-#define OPTION(type, name, defvalue) { #name, \
-	offsetof(struct luaL_serializer, name), type, defvalue}
-/**
- * Configuration options for serializers
- * @sa struct luaL_serializer
- */
-static struct {
-	const char *name;
-	size_t offset; /* offset in structure */
-	int type;
-	int defvalue;
-} OPTIONS[] = {
-	OPTION(LUA_TBOOLEAN, encode_sparse_convert, 1),
-	OPTION(LUA_TNUMBER,  encode_sparse_ratio, 2),
-	OPTION(LUA_TNUMBER,  encode_sparse_safe, 10),
-	OPTION(LUA_TNUMBER,  encode_max_depth, 32),
-	OPTION(LUA_TBOOLEAN, encode_invalid_numbers, 1),
-	OPTION(LUA_TNUMBER,  encode_number_precision, 14),
-	OPTION(LUA_TBOOLEAN, encode_load_metatables, 1),
-	OPTION(LUA_TBOOLEAN, encode_use_tostring, 0),
-	OPTION(LUA_TBOOLEAN, encode_invalid_as_nil, 0),
-	OPTION(LUA_TBOOLEAN, decode_invalid_numbers, 1),
-	OPTION(LUA_TBOOLEAN, decode_save_metatables, 1),
-	OPTION(LUA_TNUMBER,  decode_max_depth, 32),
-	{ NULL, 0, 0, 0},
-};
-
 /**
  * @brief serializer.cfg{} Lua binding for serializers.
  * serializer.cfg is a table that contains current configuration values from
@@ -224,6 +196,33 @@ static struct {
  * @param L lua stack
  * @return 0
  */
+
+/*Serialize one option. Returns ponter to the value of option.*/
+int* parse_option(lua_State *L, int i, struct luaL_serializer* cfg) {
+	lua_getfield(L, 2, OPTIONS[i].name);
+	if (lua_isnil(L, -1)) {
+		lua_pop(L, 1); /* key hasn't changed */
+		return NULL;
+	}
+	/*
+	 * Update struct luaL_serializer using pointer to a
+	 * configuration value (all values must be `int` for that).
+	 */
+	int *pval = (int *) ((char *) cfg + OPTIONS[i].offset);
+	/* Update struct luaL_serializer structure */
+	switch (OPTIONS[i].type) {
+	case LUA_TBOOLEAN:
+		*pval = lua_toboolean(L, -1);
+		break;
+	case LUA_TNUMBER:
+		*pval = lua_tointeger(L, -1);
+		break;
+	default:
+		unreachable();
+	}
+	return pval;
+}
+
 static int
 luaL_serializer_cfg(lua_State *L)
 {
@@ -232,31 +231,22 @@ luaL_serializer_cfg(lua_State *L)
 	struct luaL_serializer *cfg = luaL_checkserializer(L);
 	/* Iterate over all available options and checks keys in passed table */
 	for (int i = 0; OPTIONS[i].name != NULL; i++) {
-		lua_getfield(L, 2, OPTIONS[i].name);
-		if (lua_isnil(L, -1)) {
-			lua_pop(L, 1); /* key hasn't changed */
-			continue;
-		}
-		/*
-		 * Update struct luaL_serializer using pointer to a
-		 * configuration value (all values must be `int` for that).
-		 */
-		int *pval = (int *) ((char *) cfg + OPTIONS[i].offset);
+		int* pval = parse_option(L, i, cfg);
 		/* Update struct luaL_serializer structure */
-		switch (OPTIONS[i].type) {
-		case LUA_TBOOLEAN:
-			*pval = lua_toboolean(L, -1);
-			lua_pushboolean(L, *pval);
-			break;
-		case LUA_TNUMBER:
-			*pval = lua_tointeger(L, -1);
-			lua_pushinteger(L, *pval);
-			break;
-		default:
-			unreachable();
+		if (pval != NULL) {
+			switch (OPTIONS[i].type) {
+			case LUA_TBOOLEAN:
+					lua_pushboolean(L, *pval);
+				break;
+			case LUA_TNUMBER:
+				lua_pushinteger(L, *pval);
+				break;
+			default:
+				unreachable();
+			}
+			/* Save normalized value to serializer.cfg table */
+			lua_setfield(L, 1, OPTIONS[i].name);
 		}
-		/* Save normalized value to serializer.cfg table */
-		lua_setfield(L, 1, OPTIONS[i].name);
 	}
 	return 0;
 }
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 6b057af3e..4a2c3eaac 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -1,5 +1,6 @@
 #ifndef TARANTOOL_LUA_UTILS_H_INCLUDED
 #define TARANTOOL_LUA_UTILS_H_INCLUDED
+#pragma GCC diagnostic ignored "-Wunused-variable"
 /*
  * Copyright 2010-2015, Tarantool AUTHORS, please see AUTHORS file.
  *
@@ -240,6 +241,35 @@ luaL_checkserializer(struct lua_State *L) {
 		luaL_checkudata(L, lua_upvalueindex(1), LUAL_SERIALIZER);
 }
 
+#define OPTION(type, name, defvalue) { #name, \
+	offsetof(struct luaL_serializer, name), type, defvalue}
+/**
+ * Configuration options for serializers
+ * @sa struct luaL_serializer
+ */
+static struct {
+	const char *name;
+	size_t offset; /* offset in structure */
+	int type;
+	int defvalue;
+} OPTIONS[] = {
+	OPTION(LUA_TBOOLEAN, encode_sparse_convert, 1),
+	OPTION(LUA_TNUMBER,  encode_sparse_ratio, 2),
+	OPTION(LUA_TNUMBER,  encode_sparse_safe, 10),
+	OPTION(LUA_TNUMBER,  encode_max_depth, 32),
+	OPTION(LUA_TBOOLEAN, encode_invalid_numbers, 1),
+	OPTION(LUA_TNUMBER,  encode_number_precision, 14),
+	OPTION(LUA_TBOOLEAN, encode_load_metatables, 1),
+	OPTION(LUA_TBOOLEAN, encode_use_tostring, 0),
+	OPTION(LUA_TBOOLEAN, encode_invalid_as_nil, 0),
+	OPTION(LUA_TBOOLEAN, decode_invalid_numbers, 1),
+	OPTION(LUA_TBOOLEAN, decode_save_metatables, 1),
+	OPTION(LUA_TNUMBER,  decode_max_depth, 32),
+	{ NULL, 0, 0, 0},
+};
+
+int* parse_option(lua_State *l, int i, struct luaL_serializer* cfg);
+
 /** A single value on the Lua stack. */
 struct luaL_field {
 	union {
diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c
index bdfda1e79..861079f8a 100644
--- a/third_party/lua-cjson/lua_cjson.c
+++ b/third_party/lua-cjson/lua_cjson.c
@@ -417,89 +417,38 @@ static void json_append_data(lua_State *l, struct luaL_serializer *cfg,
     }
 }
 
-#define OPTION(type, name, defvalue) { #name, \
-	offsetof(struct luaL_serializer, name), type, defvalue}
-
-static struct {
-	const char *name;
-	size_t offset; /* offset in structure */
-	int type;
-	int defvalue;
-} OPTIONS[] = {
-	OPTION(LUA_TBOOLEAN, encode_sparse_convert, 1),
-	OPTION(LUA_TNUMBER,  encode_sparse_ratio, 2),
-	OPTION(LUA_TNUMBER,  encode_sparse_safe, 10),
-	OPTION(LUA_TNUMBER,  encode_max_depth, 32),
-	OPTION(LUA_TBOOLEAN, encode_invalid_numbers, 1),
-	OPTION(LUA_TNUMBER,  encode_number_precision, 14),
-	OPTION(LUA_TBOOLEAN, encode_load_metatables, 1),
-	OPTION(LUA_TBOOLEAN, encode_use_tostring, 0),
-	OPTION(LUA_TBOOLEAN, encode_invalid_as_nil, 0),
-	OPTION(LUA_TBOOLEAN, decode_invalid_numbers, 1),
-	OPTION(LUA_TBOOLEAN, decode_save_metatables, 1),
-	OPTION(LUA_TNUMBER,  decode_max_depth, 32),
-	{ NULL, 0, 0, 0},
-};
-
-/*Serialize Lua table of options to luaL_serializer struct*/
+/*Serialize Lua table of options to luaL_serializer struct.*/
 static int parse_options(lua_State *l, struct luaL_serializer* cfg) {
     for (int i = 0; OPTIONS[i].name != NULL; i++) {
-		lua_getfield(l, 2, OPTIONS[i].name);
-		if (lua_isnil(l, -1)) {
-			lua_pop(l, 1); /* key hasn't changed */
-			continue;
-		}
-		/*
-		 * Update struct luaL_serializer using pointer to a
-		 * configuration value (all values must be `int` for that).
-		 */
-		int *pval = (int *) ((char *) cfg + OPTIONS[i].offset);
-		/* Update struct luaL_serializer structure */
-		switch (OPTIONS[i].type) {
-		case LUA_TBOOLEAN:
-			*pval = lua_toboolean(l, -1);
-			lua_pop(l, 1);
-			break;
-		case LUA_TNUMBER:
-			*pval = lua_tointeger(l, -1);
-			lua_pop(l, 1);
-			break;
-		default:
-			unreachable();
-		}
-	}
-	lua_pop(l, 1);
-	return 0;
+        int *pval = parse_option(l, i, cfg);
+        /* Update struct luaL_serializer structure */
+        if (pval != NULL)
+            lua_pop(l, 1);
+    }
+    lua_pop(l, 1);
+    return 0;
 }

>>  +static int json_encode(lua_State *l) {
>>  + luaL_argcheck(l, (lua_gettop(l) == 2) || (lua_gettop(l) == 1), 1, "expected 1 or 2 arguments");
>
> 4. Out of 80 symbols.
>

diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c
index bdfda1e79..861079f8a 100644
-	luaL_argcheck(l, (lua_gettop(l) == 2) || (lua_gettop(l) == 1), 1, "expected 1 or 2 arguments");
+    luaL_argcheck(l, (lua_gettop(l) == 2) || (lua_gettop(l) == 1),
+                      1, "expected 1 or 2 arguments");

>>  - json_append_data(l, cfg, 0, &encode_buf);
>>  - json = strbuf_string(&encode_buf, &len);
>>  + char *json;
>>  + int len;
>
> 5. In the original code spaces were used, not tabs. Please,
> use spaces in this file too.
>
> 6. You do not need to annotate those arguments here. Please,
> declare them together with assigning.
>
> Not
> var name;
> name = value;
>
> But
> var name = value.
>

diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c
index bdfda1e79..861079f8a 100644
-	char *json;
-	int len;
+    int len;
+    char *json = strbuf_string(&encode_buf, &len);

>>  - lua_pushlstring(l, json, len);
>>  + /* Reuse existing buffer */
>>  + strbuf_reset(&encode_buf);
>>  + struct luaL_serializer *cfg = luaL_checkserializer(l);
>>
>>  - return 1;
>>  + /* If have second arg */
>>  + if (lua_gettop(l) == 2) {
>>  + struct luaL_serializer user_cfg = *cfg;
>>  +
>>  + parse_options(l, &user_cfg);
>>  +
>>  + json_append_data(l, &user_cfg, 0, &encode_buf);
>>  + }
>>  + /* If have not second arg */
>>  + else {
>
> 7. Please, put 'else' on the same line as }.

Fixed.

> 8. Put the dot at the end of sentence.
>
>>  +

Fixed.

> 9. Extra empty line.
>
>>  + json_append_data(l, cfg, 0, &encode_buf);
>>  + }
>>  +
>>  + json = strbuf_string(&encode_buf, &len);
>>  + lua_pushlstring(l, json, len);
>>  +
>>  + return 1;
>>    }

Fixed.

> 10. Please, add the options to json.decode. API must be symmetrical.


diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c
index bdfda1e79..861079f8a 100644
 /* ===== DECODING ===== */
@@ -1043,9 +992,17 @@ static int json_decode(lua_State *l)
     json_token_t token;
     size_t json_len;
 
-    luaL_argcheck(l, lua_gettop(l) == 1, 1, "expected 1 argument");
+    luaL_argcheck(l, (lua_gettop(l) == 2) || (lua_gettop(l) == 1),
+                      1, "expected 1 or 2 arguments");
+
+    if (lua_gettop(l) == 2) {
+        struct luaL_serializer user_cfg = *luaL_checkserializer(l);
+        parse_options(l, &user_cfg);
+        json.cfg = &user_cfg;
+    } else {
+        json.cfg = luaL_checkserializer(l);
+    }
 
-    json.cfg = luaL_checkserializer(l);
     json.data = luaL_checklstring(l, 1, &json_len);
     json.current_depth = 0;
     json.ptr = json.data;