* [Tarantool-patches] [PATCH luajit][v2] LJ_GC64: Fix lua_concat().
@ 2023-10-06 14:15 Sergey Bronnikov via Tarantool-patches
2023-10-08 14:58 ` Maxim Kokryashkin via Tarantool-patches
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-10-06 14:15 UTC (permalink / raw)
To: tarantool-patches, Sergey Kaplun, max.kokryashkin
From: Sergey Bronnikov <sergeyb@tarantool.org>
Reported by Mathias Westerdahl.
(cherry picked from commit 633f265f67f322cbe2c5fd11d3e46d968ac220f7)
Lua 5.1 Reference Manual [1] defines a function `lua_concat`, that:
> void lua_concat (lua_State *L, int n);
>
> Concatenates the n values at the top of the stack, pops them, and leaves
> the result at the top.
Without the patch `lua_concat()` behaved incorrectly with userdata with
defined `__concat` metamethod. The problem is GC64-specific.
Assuming we have three literals and a userdata with defined "__concat"
metamethod on top of the Lua stack:
1 [string]
2 [string]
3 [string]
4 [string]
5 [userdata] <--- top
On attempt to concatenate *two* items on top of the Lua stack,
`lua_concat()` concatenates *four* items and leaves result on the top:
1 [string]
2 [string][string][string][userdata] <--- top
The problem is in incorrect calculation of `n` counter in the loop in
implementation of function `lua_concat`. Without the fix `n` is equal to
3 at the end of the first iteration and therefore it goes to the next
iteration of concatenation. In the fixed implementation of
`lua_concat()` `n` is equal to 1 at the end of the first loop iteration,
decremented in a loop postcondition and breaks the loop.
The patch fixes incorrect behaviour.
1. https://www.lua.org/manual/5.1/manual.html
Sergey Bronnikov:
* added the description and the test for the problem
Part of tarantool/tarantool#8825
---
PR: https://github.com/tarantool/tarantool/pull/9176
Branch: https://github.com/tarantool/luajit/commits/ligurio/lj-881-fix-concat
Issues:
- https://github.com/LuaJIT/LuaJIT/issues/881
- https://github.com/tarantool/tarantool/issues/8825
Changes:
- fixed typos
- updated assertions (bool asserts replaced with int assertions)
- replaced userdata with struct with userdata with integer per Max's request
that's all, folks
src/lj_api.c | 2 +-
.../lj-881-fix-lua-concat.test.c | 95 +++++++++++++++++++
2 files changed, 96 insertions(+), 1 deletion(-)
create mode 100644 test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
diff --git a/src/lj_api.c b/src/lj_api.c
index 05e02029..3bacad33 100644
--- a/src/lj_api.c
+++ b/src/lj_api.c
@@ -801,7 +801,7 @@ LUA_API void lua_concat(lua_State *L, int n)
L->top -= n;
break;
}
- n -= (int)(L->top - top);
+ n -= (int)(L->top - (top - 2*LJ_FR2));
L->top = top+2;
jit_secure_call(L, top, 1+1);
L->top -= 1+LJ_FR2;
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
new file mode 100644
index 00000000..1b1f146f
--- /dev/null
+++ b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
@@ -0,0 +1,95 @@
+#include <string.h>
+
+#include "lua.h"
+#include "lauxlib.h"
+
+#include "test.h"
+#include "utils.h"
+
+/*
+ * This test demonstrates LuaJIT's incorrect behaviour, when
+ * calling `lua_concat()` with userdata with __concat metamethod.
+ * See https://github.com/LuaJIT/LuaJIT/issues/881 for details.
+ */
+
+#define MT_NAME "int"
+
+static int INFO_concat(lua_State *L)
+{
+ const char *s = luaL_checkstring(L, 1);
+ 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 mt[] =
+{
+ {"__concat", INFO_concat},
+ {NULL, NULL}
+};
+
+static int lua_concat_testcase(void *test_state)
+{
+ /* Setup. */
+ lua_State *L = test_state;
+ const int top = 4;
+
+ /* Create metatable and put it to the Lua registry. */
+ luaL_newmetatable(L, MT_NAME);
+ /* Fill metatable. */
+ luaL_register(L, 0, mt);
+ lua_pop(L, 1);
+
+ assert_int_equal(lua_gettop(L), 0);
+
+ lua_pushliteral(L, "C");
+ lua_pushliteral(L, "B");
+ lua_pushliteral(L, "A");
+
+ 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,
+ * pops them, and leaves the result at the top. If n is 1,
+ * the result is the single value on the stack; if n is 0,
+ * the result is the empty string [1].
+ *
+ * 1. https://www.lua.org/manual/5.1/manual.html
+ */
+
+ /* Concatenate two elements on top. */
+ lua_concat(L, 2);
+
+ const char *str = lua_tostring(L, -1);
+ 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. */
+ lua_settop(L, 0);
+
+ return TEST_EXIT_SUCCESS;
+}
+
+int main(void)
+{
+ lua_State *L = utils_lua_init();
+ const struct test_unit tgroup[] = {
+ test_unit_def(lua_concat_testcase),
+ };
+ const int test_result = test_run_group(tgroup, L);
+ utils_lua_close(L);
+
+ return test_result;
+}
--
2.34.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v2] LJ_GC64: Fix lua_concat().
2023-10-06 14:15 [Tarantool-patches] [PATCH luajit][v2] LJ_GC64: Fix lua_concat() Sergey Bronnikov via Tarantool-patches
@ 2023-10-08 14:58 ` Maxim Kokryashkin via Tarantool-patches
2023-10-09 14:11 ` Sergey Kaplun via Tarantool-patches
2023-10-31 16:00 ` Igor Munkin via Tarantool-patches
2 siblings, 0 replies; 7+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-10-08 14:58 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: max.kokryashkin, tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 5305 bytes --]
Hi, Sergey!
Thanks for the patch!
LGTM
--
Best regards,
Maxim Kokryashkin
>Пятница, 6 октября 2023, 17:16 +03:00 от Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
>
>From: Sergey Bronnikov < sergeyb@tarantool.org >
>
>Reported by Mathias Westerdahl.
>
>(cherry picked from commit 633f265f67f322cbe2c5fd11d3e46d968ac220f7)
>
>Lua 5.1 Reference Manual [1] defines a function `lua_concat`, that:
>
>> void lua_concat (lua_State *L, int n);
>>
>> Concatenates the n values at the top of the stack, pops them, and leaves
>> the result at the top.
>Without the patch `lua_concat()` behaved incorrectly with userdata with
>defined `__concat` metamethod. The problem is GC64-specific.
>
>Assuming we have three literals and a userdata with defined "__concat"
>metamethod on top of the Lua stack:
>
>1 [string]
>2 [string]
>3 [string]
>4 [string]
>5 [userdata] <--- top
>
>On attempt to concatenate *two* items on top of the Lua stack,
>`lua_concat()` concatenates *four* items and leaves result on the top:
>
>1 [string]
>2 [string][string][string][userdata] <--- top
>
>The problem is in incorrect calculation of `n` counter in the loop in
>implementation of function `lua_concat`. Without the fix `n` is equal to
>3 at the end of the first iteration and therefore it goes to the next
>iteration of concatenation. In the fixed implementation of
>`lua_concat()` `n` is equal to 1 at the end of the first loop iteration,
>decremented in a loop postcondition and breaks the loop.
>
>The patch fixes incorrect behaviour.
>
>1. https://www.lua.org/manual/5.1/manual.html
>
>Sergey Bronnikov:
>* added the description and the test for the problem
>
>Part of tarantool/tarantool#8825
>---
>PR: https://github.com/tarantool/tarantool/pull/9176
>Branch: https://github.com/tarantool/luajit/commits/ligurio/lj-881-fix-concat
>Issues:
>- https://github.com/LuaJIT/LuaJIT/issues/881
>- https://github.com/tarantool/tarantool/issues/8825
>
>Changes:
>- fixed typos
>- updated assertions (bool asserts replaced with int assertions)
>- replaced userdata with struct with userdata with integer per Max's request
>that's all, folks
>
> src/lj_api.c | 2 +-
> .../lj-881-fix-lua-concat.test.c | 95 +++++++++++++++++++
> 2 files changed, 96 insertions(+), 1 deletion(-)
> create mode 100644 test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
>
>diff --git a/src/lj_api.c b/src/lj_api.c
>index 05e02029..3bacad33 100644
>--- a/src/lj_api.c
>+++ b/src/lj_api.c
>@@ -801,7 +801,7 @@ LUA_API void lua_concat(lua_State *L, int n)
> L->top -= n;
> break;
> }
>- n -= (int)(L->top - top);
>+ n -= (int)(L->top - (top - 2*LJ_FR2));
> L->top = top+2;
> jit_secure_call(L, top, 1+1);
> L->top -= 1+LJ_FR2;
>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
>new file mode 100644
>index 00000000..1b1f146f
>--- /dev/null
>+++ b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
>@@ -0,0 +1,95 @@
>+#include <string.h>
>+
>+#include "lua.h"
>+#include "lauxlib.h"
>+
>+#include "test.h"
>+#include "utils.h"
>+
>+/*
>+ * This test demonstrates LuaJIT's incorrect behaviour, when
>+ * calling `lua_concat()` with userdata with __concat metamethod.
>+ * See https://github.com/LuaJIT/LuaJIT/issues/881 for details.
>+ */
>+
>+#define MT_NAME "int"
>+
>+static int INFO_concat(lua_State *L)
>+{
>+ const char *s = luaL_checkstring(L, 1);
>+ 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 mt[] =
>+{
>+ {"__concat", INFO_concat},
>+ {NULL, NULL}
>+};
>+
>+static int lua_concat_testcase(void *test_state)
>+{
>+ /* Setup. */
>+ lua_State *L = test_state;
>+ const int top = 4;
>+
>+ /* Create metatable and put it to the Lua registry. */
>+ luaL_newmetatable(L, MT_NAME);
>+ /* Fill metatable. */
>+ luaL_register(L, 0, mt);
>+ lua_pop(L, 1);
>+
>+ assert_int_equal(lua_gettop(L), 0);
>+
>+ lua_pushliteral(L, "C");
>+ lua_pushliteral(L, "B");
>+ lua_pushliteral(L, "A");
>+
>+ 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,
>+ * pops them, and leaves the result at the top. If n is 1,
>+ * the result is the single value on the stack; if n is 0,
>+ * the result is the empty string [1].
>+ *
>+ * 1. https://www.lua.org/manual/5.1/manual.html
>+ */
>+
>+ /* Concatenate two elements on top. */
>+ lua_concat(L, 2);
>+
>+ const char *str = lua_tostring(L, -1);
>+ 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. */
>+ lua_settop(L, 0);
>+
>+ return TEST_EXIT_SUCCESS;
>+}
>+
>+int main(void)
>+{
>+ lua_State *L = utils_lua_init();
>+ const struct test_unit tgroup[] = {
>+ test_unit_def(lua_concat_testcase),
>+ };
>+ const int test_result = test_run_group(tgroup, L);
>+ utils_lua_close(L);
>+
>+ return test_result;
>+}
>--
>2.34.1
[-- Attachment #2: Type: text/html, Size: 6937 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v2] LJ_GC64: Fix lua_concat().
2023-10-06 14:15 [Tarantool-patches] [PATCH luajit][v2] LJ_GC64: Fix lua_concat() Sergey Bronnikov via Tarantool-patches
2023-10-08 14:58 ` Maxim Kokryashkin via Tarantool-patches
@ 2023-10-09 14:11 ` Sergey Kaplun via Tarantool-patches
2023-10-10 12:33 ` Sergey Bronnikov via Tarantool-patches
2023-10-31 16:00 ` Igor Munkin via Tarantool-patches
2 siblings, 1 reply; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-10-09 14:11 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: max.kokryashkin, tarantool-patches
Hi, Sergey!
Thanks for the patch!
LGTM, after fixing a few nits mentioned below.
On 06.10.23, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
>
> Reported by Mathias Westerdahl.
>
> (cherry picked from commit 633f265f67f322cbe2c5fd11d3e46d968ac220f7)
>
> Lua 5.1 Reference Manual [1] defines a function `lua_concat`, that:
>
> > void lua_concat (lua_State *L, int n);
> >
> > Concatenates the n values at the top of the stack, pops them, and leaves
> > the result at the top.
>
> Without the patch `lua_concat()` behaved incorrectly with userdata with
Typo: s/patch/patch,/
> defined `__concat` metamethod. The problem is GC64-specific.
Typo: s/defined/the defined/
>
> Assuming we have three literals and a userdata with defined "__concat"
Typo: s/defined/the defined/
> metamethod on top of the Lua stack:
>
> 1 [string]
> 2 [string]
> 3 [string]
> 4 [string]
> 5 [userdata] <--- top
>
> On attempt to concatenate *two* items on top of the Lua stack,
> `lua_concat()` concatenates *four* items and leaves result on the top:
Typo: s/result/the result/
Typo: s/the top/top of the Lua stack/
or s/on the top/at the top of the Lua stack/
>
> 1 [string]
> 2 [string][string][string][userdata] <--- top
>
> The problem is in incorrect calculation of `n` counter in the loop in
Typo: s/in incorrect/in the incorrect/
> implementation of function `lua_concat`. Without the fix `n` is equal to
Typo: s/in implementation/in the implementation/
Typo: s/fix/fix,/
> 3 at the end of the first iteration and therefore it goes to the next
Typo: s/ and/, and/
> iteration of concatenation. In the fixed implementation of
> `lua_concat()` `n` is equal to 1 at the end of the first loop iteration,
Typo: s/`lua_concat()`/`lua_concat()`,/
> decremented in a loop postcondition and breaks the loop.
Typo: s/ and/, and/
>
Minor: Also, we can mention that for details, the reader can see
`lj_meta_cat()` implementation in <src/lj_meta.c>.
Feel free to ignore.
> The patch fixes incorrect behaviour.
>
> 1. https://www.lua.org/manual/5.1/manual.html
>
> Sergey Bronnikov:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#8825
I suppose this should be #9145.
> ---
> PR: https://github.com/tarantool/tarantool/pull/9176
> Branch: https://github.com/tarantool/luajit/commits/ligurio/lj-881-fix-concat
> Issues:
> - https://github.com/LuaJIT/LuaJIT/issues/881
> - https://github.com/tarantool/tarantool/issues/8825
<snipped>
> 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
> new file mode 100644
> index 00000000..1b1f146f
> --- /dev/null
> +++ b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
> @@ -0,0 +1,95 @@
> +#include <string.h>
> +
> +#include "lua.h"
> +#include "lauxlib.h"
> +
> +#include "test.h"
> +#include "utils.h"
> +
> +/*
> + * This test demonstrates LuaJIT's incorrect behaviour, when
Typo: s/,//
> + * calling `lua_concat()` with userdata with __concat metamethod.
Typo: s/__concat/the __concat/
> + * See https://github.com/LuaJIT/LuaJIT/issues/881 for details.
> + */
> +
> +#define MT_NAME "int"
> +
> +static int INFO_concat(lua_State *L)
> +{
> + const char *s = luaL_checkstring(L, 1);
> + 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 mt[] =
> +{
Minor: I suppose this line may be joined with the previous one, as far
as this is not a function's body.
> + {"__concat", INFO_concat},
Typo: s/, /, /
> + {NULL, NULL}
> +};
> +
> +static int lua_concat_testcase(void *test_state)
> +{
<snipped>
> +
> + int *n = (int *)lua_newuserdata(L, sizeof(int));
I suggest to use `sizeof(*n)` here to avoid type duplication.
> + *n = 100;
I suppose we can use something like the following:
| #define TEST_VALUE 100
| #define STR(s) #s
| #define XSTR(s) STR(s)
| #define TEST_VALUE_STR XSTR(TEST_VALUE)
or just use #TEST_VALUE in the `expected_str[]` declaration.
This helps to avoid the duplication of the magic number `100`.
> +
<snipped>
> + const char expected_str[] = "A + 100";
> + assert_str_equal(str, expected_str, strlen(expected_str));
<snipped>
> --
> 2.34.1
>
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v2] LJ_GC64: Fix lua_concat().
2023-10-09 14:11 ` Sergey Kaplun via Tarantool-patches
@ 2023-10-10 12:33 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 0 replies; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-10-10 12:33 UTC (permalink / raw)
To: Sergey Kaplun, Sergey Bronnikov; +Cc: tarantool-patches, max.kokryashkin
Hi, Sergey
On 09.10.2023 17:11, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, after fixing a few nits mentioned below.
>
> On 06.10.23, Sergey Bronnikov wrote:
>> From: Sergey Bronnikov <sergeyb@tarantool.org>
>>
>> Reported by Mathias Westerdahl.
>>
>> (cherry picked from commit 633f265f67f322cbe2c5fd11d3e46d968ac220f7)
>>
>> Lua 5.1 Reference Manual [1] defines a function `lua_concat`, that:
>>
>>> void lua_concat (lua_State *L, int n);
>>>
>>> Concatenates the n values at the top of the stack, pops them, and leaves
>>> the result at the top.
>> Without the patch `lua_concat()` behaved incorrectly with userdata with
> Typo: s/patch/patch,/
Fixed.
>
>> defined `__concat` metamethod. The problem is GC64-specific.
> Typo: s/defined/the defined/
Fixed.
>
>> Assuming we have three literals and a userdata with defined "__concat"
> Typo: s/defined/the defined/
Fixed.
>
>> metamethod on top of the Lua stack:
>>
>> 1 [string]
>> 2 [string]
>> 3 [string]
>> 4 [string]
>> 5 [userdata] <--- top
>>
>> On attempt to concatenate *two* items on top of the Lua stack,
>> `lua_concat()` concatenates *four* items and leaves result on the top:
> Typo: s/result/the result/
> Typo: s/the top/top of the Lua stack/
> or s/on the top/at the top of the Lua stack/
Fixed.
>
>> 1 [string]
>> 2 [string][string][string][userdata] <--- top
>>
>> The problem is in incorrect calculation of `n` counter in the loop in
> Typo: s/in incorrect/in the incorrect/
Fixed.
>
>> implementation of function `lua_concat`. Without the fix `n` is equal to
> Typo: s/in implementation/in the implementation/
> Typo: s/fix/fix,/
Fixed.
>
>> 3 at the end of the first iteration and therefore it goes to the next
> Typo: s/ and/, and/
Fixed.
>
>> iteration of concatenation. In the fixed implementation of
>> `lua_concat()` `n` is equal to 1 at the end of the first loop iteration,
> Typo: s/`lua_concat()`/`lua_concat()`,/
Fixed.
>
>> decremented in a loop postcondition and breaks the loop.
> Typo: s/ and/, and/
Fixed.
> Minor: Also, we can mention that for details, the reader can see
> `lj_meta_cat()` implementation in <src/lj_meta.c>.
> Feel free to ignore.
Added.
>> The patch fixes incorrect behaviour.
>>
>> 1. https://www.lua.org/manual/5.1/manual.html
>>
>> Sergey Bronnikov:
>> * added the description and the test for the problem
>>
>> Part of tarantool/tarantool#8825
> I suppose this should be #9145.
Done.
>
>> ---
>> PR: https://github.com/tarantool/tarantool/pull/9176
>> Branch: https://github.com/tarantool/luajit/commits/ligurio/lj-881-fix-concat
>> Issues:
>> - https://github.com/LuaJIT/LuaJIT/issues/881
>> - https://github.com/tarantool/tarantool/issues/8825
> <snipped>
>
>> 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
>> new file mode 100644
>> index 00000000..1b1f146f
>> --- /dev/null
>> +++ b/test/tarantool-c-tests/lj-881-fix-lua-concat.test.c
>> @@ -0,0 +1,95 @@
>> +#include <string.h>
>> +
>> +#include "lua.h"
>> +#include "lauxlib.h"
>> +
>> +#include "test.h"
>> +#include "utils.h"
>> +
>> +/*
>> + * This test demonstrates LuaJIT's incorrect behaviour, when
> Typo: s/,//
Fixed.
>
>> + * calling `lua_concat()` with userdata with __concat metamethod.
> Typo: s/__concat/the __concat/
Fixed.
>
>> + * See https://github.com/LuaJIT/LuaJIT/issues/881 for details.
>> + */
>> +
>> +#define MT_NAME "int"
>> +
>> +static int INFO_concat(lua_State *L)
>> +{
>> + const char *s = luaL_checkstring(L, 1);
>> + 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 mt[] =
>> +{
> Minor: I suppose this line may be joined with the previous one, as far
> as this is not a function's body.
Joined.
>
>> + {"__concat", INFO_concat},
> Typo: s/, /, /
Fixed.
>
>> + {NULL, NULL}
>> +};
>> +
>> +static int lua_concat_testcase(void *test_state)
>> +{
> <snipped>
>
>> +
>> + int *n = (int *)lua_newuserdata(L, sizeof(int));
> I suggest to use `sizeof(*n)` here to avoid type duplication.
Fixed.
>
>> + *n = 100;
> I suppose we can use something like the following:
>
> | #define TEST_VALUE 100
> | #define STR(s) #s
> | #define XSTR(s) STR(s)
> | #define TEST_VALUE_STR XSTR(TEST_VALUE)
>
> or just use #TEST_VALUE in the `expected_str[]` declaration.
> This helps to avoid the duplication of the magic number `100`.
Updated:
#define TYPE_NAME "int"
+#define TEST_VALUE 100
+#define TOSTR(s) #s
+#define CONCAT(A, B) A TOSTR(B)
+
static int __concat(lua_State *L)
{
const char *s = luaL_checkstring(L, 1);
@@ -73,7 +76,7 @@ static int lua_concat_testcase(void *test_state)
const char *str = lua_tostring(L, -1);
assert_int_equal(lua_gettop(L), top - 2 + 1);
- const char expected_str[] = "A + 100";
+ const char expected_str[] = CONCAT("A + ", TEST_VALUE);
assert_str_equal(str, expected_str, strlen(expected_str));
/* Teardown. */
>
>> +
> <snipped>
>
>> + const char expected_str[] = "A + 100";
>> + assert_str_equal(str, expected_str, strlen(expected_str));
> <snipped>
>
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v2] LJ_GC64: Fix lua_concat().
2023-10-06 14:15 [Tarantool-patches] [PATCH luajit][v2] LJ_GC64: Fix lua_concat() Sergey Bronnikov via Tarantool-patches
2023-10-08 14:58 ` Maxim Kokryashkin via Tarantool-patches
2023-10-09 14:11 ` Sergey Kaplun via Tarantool-patches
@ 2023-10-31 16:00 ` Igor Munkin via Tarantool-patches
2023-10-31 18:14 ` Sergey Bronnikov via Tarantool-patches
2 siblings, 1 reply; 7+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2023-10-31 16:00 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: max.kokryashkin, tarantool-patches
Sergey,
I didn't find the patch where <assert_str_equal>/<assert_str_not_equal>
are introduced. Furthermore, I'd rather recommend to introduce
<assert_str0_equal>/<assert_str0_not_equal> comparing nul-terminated
strings, so length is not required (it's just a simple wrapper around
the functions you've already implemented).
Anyway, I'd like to ask you to send the aforementioned patch.
--
Best regards,
IM
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v2] LJ_GC64: Fix lua_concat().
2023-10-31 16:00 ` Igor Munkin via Tarantool-patches
@ 2023-10-31 18:14 ` Sergey Bronnikov via Tarantool-patches
2023-10-31 19:22 ` Igor Munkin via Tarantool-patches
0 siblings, 1 reply; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-10-31 18:14 UTC (permalink / raw)
To: Igor Munkin, Sergey Bronnikov; +Cc: max.kokryashkin, tarantool-patches
Hello, Igor
On 10/31/23 19:00, Igor Munkin via Tarantool-patches wrote:
> Sergey,
>
> I didn't find the patch where <assert_str_equal>/<assert_str_not_equal>
> are introduced. Furthermore, I'd rather recommend to introduce
> <assert_str0_equal>/<assert_str0_not_equal> comparing nul-terminated
> strings, so length is not required (it's just a simple wrapper around
> the functions you've already implemented).
Sent the current version of the patch [1].
1.
https://lists.tarantool.org/tarantool-patches/a860626525b4c5c6c82f1983d8ed9ffdb20a5a30.1698775628.git.sergeyb@tarantool.org/
>
> Anyway, I'd like to ask you to send the aforementioned patch.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v2] LJ_GC64: Fix lua_concat().
2023-10-31 18:14 ` Sergey Bronnikov via Tarantool-patches
@ 2023-10-31 19:22 ` Igor Munkin via Tarantool-patches
0 siblings, 0 replies; 7+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2023-10-31 19:22 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: Sergey Bronnikov, max.kokryashkin, tarantool-patches
Sergey,
On 31.10.23, Sergey Bronnikov wrote:
> Hello, Igor
>
> On 10/31/23 19:00, Igor Munkin via Tarantool-patches wrote:
> > Sergey,
> >
> > I didn't find the patch where <assert_str_equal>/<assert_str_not_equal>
> > are introduced. Furthermore, I'd rather recommend to introduce
> > <assert_str0_equal>/<assert_str0_not_equal> comparing nul-terminated
> > strings, so length is not required (it's just a simple wrapper around
> > the functions you've already implemented).
>
> Sent the current version of the patch [1].
Well... Now I get, how I lost it, thanks!
>
>
> 1. https://lists.tarantool.org/tarantool-patches/a860626525b4c5c6c82f1983d8ed9ffdb20a5a30.1698775628.git.sergeyb@tarantool.org/
>
> >
> > Anyway, I'd like to ask you to send the aforementioned patch.
> >
--
Best regards,
IM
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-10-31 19:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-06 14:15 [Tarantool-patches] [PATCH luajit][v2] LJ_GC64: Fix lua_concat() Sergey Bronnikov via Tarantool-patches
2023-10-08 14:58 ` Maxim Kokryashkin via Tarantool-patches
2023-10-09 14:11 ` Sergey Kaplun via Tarantool-patches
2023-10-10 12:33 ` Sergey Bronnikov via Tarantool-patches
2023-10-31 16:00 ` Igor Munkin via Tarantool-patches
2023-10-31 18:14 ` Sergey Bronnikov via Tarantool-patches
2023-10-31 19:22 ` Igor Munkin via Tarantool-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox