Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix lua_concat().

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Max

On 10/4/23 13:48, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, except for the single comment below.
>   > > +	Info info;
>>>> +	info.value = 7;
> I am really sorry, that I didn't noticed that in the first round,
> but the test case can be reduced a bit more. We don't really need
> the `Info` structure and can use some basic type like `int`, for
> example. One might argue that such change can reduce the readability
> a bit, so I leave the final decision up to you.
> <snipped>
>   
>
Thanks for suggestion, it has reduced test a bit.

Below is a patch where struct was replaced with int (changes 
force-pushed and I'll send v2 as you requested):


diff --git a/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c 
b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
index 83925445..1b1f146f 100644
--- a/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
+++ b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
@@ -12,31 +12,18 @@
   * See https://github.com/LuaJIT/LuaJIT/issues/881 for details.
   */

-#define TYPE_NAME_INFO "Info"
-
-typedef struct Info
-{
-    int payload;
-} Info;
-
-static void lua_pushinfo(lua_State *L, Info *info)
-{
-    Info *infop = (Info *)lua_newuserdata(L, sizeof(Info));
-    *infop = *info;
-
-    luaL_getmetatable(L, TYPE_NAME_INFO);
-    lua_setmetatable(L, -2);
-}
+#define MT_NAME "int"

  static int INFO_concat(lua_State *L)
  {
      const char *s = luaL_checkstring(L, 1);
-    Info *info = (Info*)luaL_checkudata(L, 2, TYPE_NAME_INFO);
-    lua_pushfstring(L, "%s[%s.payload=%d]", s, TYPE_NAME_INFO, 
info->payload);
+    int *n = (int *)luaL_checkudata(L, 2, MT_NAME);
+    /* Do non-default concatenation. */
+    lua_pushfstring(L, "%s + %d", s, *n);
      return 1;
  }

-static const luaL_Reg INFO_meta[] =
+static const luaL_Reg mt[] =
  {
      {"__concat",    INFO_concat},
      {NULL, NULL}
@@ -46,17 +33,12 @@ static int lua_concat_testcase(void *test_state)
  {
      /* Setup. */
      lua_State *L = test_state;
-    Info info = {
-        .payload = 7
-    };
      const int top = 4;
-    const int n = 2;
-
-    /* Create metatable and add it to the Lua registry. */
-    luaL_newmetatable(L, TYPE_NAME_INFO);

+    /* Create metatable and put it to the Lua registry. */
+    luaL_newmetatable(L, MT_NAME);
      /* Fill metatable. */
-    luaL_register(L, 0, INFO_meta);
+    luaL_register(L, 0, mt);
      lua_pop(L, 1);

      assert_int_equal(lua_gettop(L), 0);
@@ -64,13 +46,18 @@ static int lua_concat_testcase(void *test_state)
      lua_pushliteral(L, "C");
      lua_pushliteral(L, "B");
      lua_pushliteral(L, "A");
-    lua_pushinfo(L, &info);

-    assert_true(lua_gettop(L) == top);
+    int *n = (int *)lua_newuserdata(L, sizeof(int));
+    *n = 100;
+
+    luaL_getmetatable(L, MT_NAME);
+    lua_setmetatable(L, -2);
+
+    assert_int_equal(lua_gettop(L), top);

      /* Test body. */

-    /**
+    /*
       * void lua_concat (lua_State *L, int n);
       *
       * Concatenates the n values at the top of the stack,
@@ -80,11 +67,13 @@ static int lua_concat_testcase(void *test_state)
       *
       * 1. https://www.lua.org/manual/5.1/manual.html
       */
-    lua_concat(L, n);
+
+    /* Concatenate two elements on top. */
+    lua_concat(L, 2);

      const char *str = lua_tostring(L, -1);
-    assert_true(top - n + 1 == lua_gettop(L));
-    const char expected_str[] = "A[Info.payload=7]";
+    assert_int_equal(lua_gettop(L), top - 2 + 1);
+    const char expected_str[] = "A + 100";
      assert_str_equal(str, expected_str, strlen(expected_str));

      /* Teardown. */


<snipped>