Tarantool development patches archive
 help / color / mirror / Atom feed
* [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