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

  • From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
  • To: roman.habibov1@xxxxxxxxx, "tarantool-patches@xxxxxxxxxxxxx" <tarantool-patches@xxxxxxxxxxxxx>
  • Date: Thu, 19 Jul 2018 13:18:37 +0300

Hi! Thanks for the fixes! Please, put a new version of
the patch at the end of email sending the former. Now I
did it for you.

See 10 comments below.

commit a229af626a78c7f37b628101e189f7727c01ccd8
Author: Roman Khabibov <roman.habibov1@xxxxxxxxx>
Date:   Sun Jul 8 02:21:08 2018 +0300

    json: add options to json.encode()
Add an ability to pass options to json.encode()/decode().
Closes: #2888.

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}

1. You should not touch this hunk. Utils should provide
only API to configure the serializer, and do not expose
options array.

-/**
- * 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
  */

2. You've cut the comment off. It belongs to another function.

+
+/*Serialize one option. Returns ponter to the value of option.*/
+int* parse_option(lua_State *L, int i, struct luaL_serializer* cfg) {

3. Bad style. Please, see in other functions how to write comments
and how to declare pointers. You should not write int*, but int *.
Please, wrap line after the function return type declaring.

+       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);

4. Broken indentation.

+                               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"

5. Garbage diff. You should never change compilation
options via pragma. And if you need this one, then your
patch has a problem, that should be fixed, not hidden.

 /*
  * 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);

6. Please, write function comment on top of the declaration. Not on
top of the implementation.

+
 /** A single value on the Lua stack. */
 struct luaL_field {
        union {
diff --git a/test/app-tap/json.test.lua b/test/app-tap/json.test.lua
index 3884b41e7..050d769ea 100755
--- a/test/app-tap/json.test.lua
+++ b/test/app-tap/json.test.lua
@@ -22,7 +22,42 @@ end
tap.test("json", function(test)
     local serializer = require('json')
-    test:plan(9)
+    test:plan(18)
+
+-- gh-2888 Added opions to encode().

7. Typo: opions.

8. Please, consult other test files how to write a test comment.
It should not be a description of what was done, but an explanation
of the problem.

9. I do not see tests on decode.

diff --git a/third_party/lua-cjson/lua_cjson.c 
b/third_party/lua-cjson/lua_cjson.c
index aa8217dfb..861079f8a 100644
--- a/third_party/lua-cjson/lua_cjson.c
+++ b/third_party/lua-cjson/lua_cjson.c
@@ -417,22 +417,37 @@ 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;
+/*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++) {
+        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;
+}
- luaL_argcheck(l, lua_gettop(l) == 1, 1, "expected 1 argument");
+static int json_encode(lua_State *l) {
+    luaL_argcheck(l, (lua_gettop(l) == 2) || (lua_gettop(l) == 1),
+                      1, "expected 1 or 2 arguments");

10. Bad indentation. In decode too.

/* Reuse existing buffer */
     strbuf_reset(&encode_buf);
+    struct luaL_serializer *cfg = luaL_checkserializer(l);
- json_append_data(l, cfg, 0, &encode_buf);
-    json = strbuf_string(&encode_buf, &len);
+    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);
+    } else {
+        json_append_data(l, cfg, 0, &encode_buf);
+}
+ int len;
+    char *json = strbuf_string(&encode_buf, &len);
     lua_pushlstring(l, json, len);
-
     return 1;
 }


Other related posts: